bug,entity: use a dedicated type to store IDs

10 files changed, 85 insertions, 71 deletions

diff --git a/identity/bare.go b/identity/bare.go
index 6af794dd..93cea91f 100644
--- a/identity/bare.go
+++ b/identity/bare.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/lamport"
 	"github.com/MichaelMure/git-bug/util/text"
@@ -13,6 +14,7 @@ import (
 )
 
 var _ Interface = &Bare{}
+var _ entity.Interface = &Bare{}
 
 // Bare is a very minimal identity, designed to be fully embedded directly along
 // other data.
@@ -20,7 +22,7 @@ var _ Interface = &Bare{}
 // in particular, this identity is designed to be compatible with the handling of
 // identities in the early version of git-bug.
 type Bare struct {
-	id        string
+	id        entity.Id
 	name      string
 	email     string
 	login     string
@@ -28,11 +30,16 @@ type Bare struct {
 }
 
 func NewBare(name string, email string) *Bare {
-	return &Bare{name: name, email: email}
+	return &Bare{id: entity.UnsetId, name: name, email: email}
 }
 
 func NewBareFull(name string, email string, login string, avatarUrl string) *Bare {
-	return &Bare{name: name, email: email, login: login, avatarUrl: avatarUrl}
+	return &Bare{id: entity.UnsetId, name: name, email: email, login: login, avatarUrl: avatarUrl}
+}
+
+func deriveId(data []byte) entity.Id {
+	sum := sha256.Sum256(data)
+	return entity.Id(fmt.Sprintf("%x", sum))
 }
 
 type bareIdentityJSON struct {
@@ -43,15 +50,19 @@ type bareIdentityJSON struct {
 }
 
 func (i *Bare) MarshalJSON() ([]byte, error) {
-	return json.Marshal(bareIdentityJSON{
+	data, err := json.Marshal(bareIdentityJSON{
 		Name:      i.name,
 		Email:     i.email,
 		Login:     i.login,
 		AvatarUrl: i.avatarUrl,
 	})
+	return data, err
 }
 
 func (i *Bare) UnmarshalJSON(data []byte) error {
+	// Compute the Id when loading the op from disk.
+	i.id = deriveId(data)
+
 	aux := bareIdentityJSON{}
 
 	if err := json.Unmarshal(data, &aux); err != nil {
@@ -67,30 +78,28 @@ func (i *Bare) UnmarshalJSON(data []byte) error {
 }
 
 // Id return the Identity identifier
-func (i *Bare) Id() string {
-	// We don't have a proper ID at hand, so let's hash all the data to get one.
-	// Hopefully the
-
-	if i.id != "" {
-		return i.id
-	}
+func (i *Bare) Id() entity.Id {
+	// We don't have a proper Id at hand, so let's hash all the data to get one.
 
-	data, err := json.Marshal(i)
-	if err != nil {
-		panic(err)
+	if i.id == "" {
+		// something went really wrong
+		panic("identity's id not set")
 	}
+	if i.id == entity.UnsetId {
+		// This means we are trying to get the identity identifier *before* it has been stored
+		// As the Id is computed based on the actual bytes written on the disk, we are going to predict
+		// those and then get the Id. This is safe as it will be the exact same code writing on disk later.
 
-	h := fmt.Sprintf("%x", sha256.New().Sum(data)[:16])
-	i.id = string(h)
+		data, err := json.Marshal(i)
+		if err != nil {
+			panic(err)
+		}
 
+		i.id = deriveId(data)
+	}
 	return i.id
 }
 
-// HumanId return the Identity identifier truncated for human consumption
-func (i *Bare) HumanId() string {
-	return FormatHumanID(i.Id())
-}
-
 // Name return the last version of the name
 func (i *Bare) Name() string {
 	return i.name
diff --git a/identity/bare_test.go b/identity/bare_test.go
index 7db9f644..335c8d37 100644
--- a/identity/bare_test.go
+++ b/identity/bare_test.go
@@ -5,12 +5,15 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/MichaelMure/git-bug/entity"
 )
 
 func TestBare_Id(t *testing.T) {
 	i := NewBare("name", "email")
 	id := i.Id()
-	assert.Equal(t, "7b226e616d65223a226e616d65222c22", id)
+	expected := entity.Id("e18b853fbd89d5d40ca24811539c9a800c705abd9232f396954e8ca8bb63fa8a")
+	assert.Equal(t, expected, id)
 }
 
 func TestBareSerialize(t *testing.T) {
@@ -28,5 +31,7 @@ func TestBareSerialize(t *testing.T) {
 	err = json.Unmarshal(data, &after)
 	assert.NoError(t, err)
 
+	before.id = after.id
+
 	assert.Equal(t, before, &after)
 }
diff --git a/identity/common.go b/identity/common.go
index 2f2b9042..68a6d57c 100644
--- a/identity/common.go
+++ b/identity/common.go
@@ -37,11 +37,11 @@ func UnmarshalJSON(raw json.RawMessage) (Interface, error) {
 	}
 
 	// Fallback on a legacy Bare identity
-	var b Bare
+	b := &Bare{}
 
-	err = json.Unmarshal(raw, &b)
+	err = json.Unmarshal(raw, b)
 	if err == nil && (b.name != "" || b.login != "") {
-		return &b, nil
+		return b, nil
 	}
 
 	// abort if we have an error other than the wrong type
diff --git a/identity/identity.go b/identity/identity.go
index f952bbf9..765b77cd 100644
--- a/identity/identity.go
+++ b/identity/identity.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/pkg/errors"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/git"
 	"github.com/MichaelMure/git-bug/util/lamport"
@@ -21,18 +22,16 @@ const identityRemoteRefPattern = "refs/remotes/%s/identities/"
 const versionEntryName = "version"
 const identityConfigKey = "git-bug.identity"
 
-const idLength = 40
-const humanIdLength = 7
-
 var ErrNonFastForwardMerge = errors.New("non fast-forward identity merge")
 var ErrNoIdentitySet = errors.New("to interact with bugs, an identity first needs to be created using \"git bug user create\" or \"git bug user adopt\"")
 var ErrMultipleIdentitiesSet = errors.New("multiple user identities set")
 
 var _ Interface = &Identity{}
+var _ entity.Interface = &Identity{}
 
 type Identity struct {
 	// Id used as unique identifier
-	id string
+	id entity.Id
 
 	// all the successive version of the identity
 	versions []*Version
@@ -43,6 +42,7 @@ type Identity struct {
 
 func NewIdentity(name string, email string) *Identity {
 	return &Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				name:  name,
@@ -55,6 +55,7 @@ func NewIdentity(name string, email string) *Identity {
 
 func NewIdentityFull(name string, email string, login string, avatarUrl string) *Identity {
 	return &Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				name:      name,
@@ -70,7 +71,7 @@ func NewIdentityFull(name string, email string, login string, avatarUrl string)
 // MarshalJSON will only serialize the id
 func (i *Identity) MarshalJSON() ([]byte, error) {
 	return json.Marshal(&IdentityStub{
-		id: i.Id(),
+		id: i.id,
 	})
 }
 
@@ -82,7 +83,7 @@ func (i *Identity) UnmarshalJSON(data []byte) error {
 }
 
 // ReadLocal load a local Identity from the identities data available in git
-func ReadLocal(repo repository.Repo, id string) (*Identity, error) {
+func ReadLocal(repo repository.Repo, id entity.Id) (*Identity, error) {
 	ref := fmt.Sprintf("%s%s", identityRefPattern, id)
 	return read(repo, ref)
 }
@@ -96,10 +97,10 @@ func ReadRemote(repo repository.Repo, remote string, id string) (*Identity, erro
 // read will load and parse an identity from git
 func read(repo repository.Repo, ref string) (*Identity, error) {
 	refSplit := strings.Split(ref, "/")
-	id := refSplit[len(refSplit)-1]
+	id := entity.Id(refSplit[len(refSplit)-1])
 
-	if len(id) != idLength {
-		return nil, fmt.Errorf("invalid ref length")
+	if err := id.Validate(); err != nil {
+		return nil, errors.Wrap(err, "invalid ref")
 	}
 
 	hashes, err := repo.ListCommits(ref)
@@ -233,7 +234,7 @@ func IsUserIdentitySet(repo repository.RepoCommon) (bool, error) {
 
 // SetUserIdentity store the user identity's id in the git config
 func SetUserIdentity(repo repository.RepoCommon, identity *Identity) error {
-	return repo.StoreConfig(identityConfigKey, identity.Id())
+	return repo.StoreConfig(identityConfigKey, identity.Id().String())
 }
 
 // GetUserIdentity read the current user identity, set with a git config entry
@@ -251,9 +252,13 @@ func GetUserIdentity(repo repository.Repo) (*Identity, error) {
 		return nil, ErrMultipleIdentitiesSet
 	}
 
-	var id string
+	var id entity.Id
 	for _, val := range configs {
-		id = val
+		id = entity.Id(val)
+	}
+
+	if err := id.Validate(); err != nil {
+		return nil, err
 	}
 
 	i, err := ReadLocal(repo, id)
@@ -326,8 +331,8 @@ func (i *Identity) Commit(repo repository.ClockedRepo) error {
 		v.commitHash = commitHash
 
 		// if it was the first commit, use the commit hash as the Identity id
-		if i.id == "" {
-			i.id = string(commitHash)
+		if i.id == "" || i.id == entity.UnsetId {
+			i.id = entity.Id(commitHash)
 		}
 	}
 
@@ -410,7 +415,7 @@ func (i *Identity) Merge(repo repository.Repo, other *Identity) (bool, error) {
 	}
 
 	if modified {
-		err := repo.UpdateRef(identityRefPattern+i.id, i.lastCommit)
+		err := repo.UpdateRef(identityRefPattern+i.id.String(), i.lastCommit)
 		if err != nil {
 			return false, err
 		}
@@ -439,8 +444,8 @@ func (i *Identity) Validate() error {
 		lastTime = v.time
 	}
 
-	// The identity ID should be the hash of the first commit
-	if i.versions[0].commitHash != "" && string(i.versions[0].commitHash) != i.id {
+	// The identity Id should be the hash of the first commit
+	if i.versions[0].commitHash != "" && string(i.versions[0].commitHash) != i.id.String() {
 		return fmt.Errorf("identity id should be the first commit hash")
 	}
 
@@ -456,7 +461,7 @@ func (i *Identity) lastVersion() *Version {
 }
 
 // Id return the Identity identifier
-func (i *Identity) Id() string {
+func (i *Identity) Id() entity.Id {
 	if i.id == "" {
 		// simply panic as it would be a coding error
 		// (using an id of an identity not stored yet)
@@ -465,16 +470,6 @@ func (i *Identity) Id() string {
 	return i.id
 }
 
-// HumanId return the Identity identifier truncated for human consumption
-func (i *Identity) HumanId() string {
-	return FormatHumanID(i.Id())
-}
-
-func FormatHumanID(id string) string {
-	format := fmt.Sprintf("%%.%ds", humanIdLength)
-	return fmt.Sprintf(format, id)
-}
-
 // Name return the last version of the name
 func (i *Identity) Name() string {
 	return i.lastVersion().name
diff --git a/identity/identity_actions.go b/identity/identity_actions.go
index 38d83b19..40aab8e2 100644
--- a/identity/identity_actions.go
+++ b/identity/identity_actions.go
@@ -75,7 +75,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes
 				continue
 			}
 
-			localRef := identityRefPattern + remoteIdentity.Id()
+			localRef := identityRefPattern + remoteIdentity.Id().String()
 			localExist, err := repo.RefExist(localRef)
 
 			if err != nil {
diff --git a/identity/identity_stub.go b/identity/identity_stub.go
index a2cee0ad..be52ffc0 100644
--- a/identity/identity_stub.go
+++ b/identity/identity_stub.go
@@ -3,6 +3,7 @@ package identity
 import (
 	"encoding/json"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/lamport"
 	"github.com/MichaelMure/git-bug/util/timestamp"
@@ -16,13 +17,13 @@ var _ Interface = &IdentityStub{}
 // When this JSON is deserialized, an IdentityStub is returned instead, to be replaced
 // later by the proper Identity, loaded from the Repo.
 type IdentityStub struct {
-	id string
+	id entity.Id
 }
 
 func (i *IdentityStub) MarshalJSON() ([]byte, error) {
 	// TODO: add a type marker
 	return json.Marshal(struct {
-		Id string `json:"id"`
+		Id entity.Id `json:"id"`
 	}{
 		Id: i.id,
 	})
@@ -30,7 +31,7 @@ func (i *IdentityStub) MarshalJSON() ([]byte, error) {
 
 func (i *IdentityStub) UnmarshalJSON(data []byte) error {
 	aux := struct {
-		Id string `json:"id"`
+		Id entity.Id `json:"id"`
 	}{}
 
 	if err := json.Unmarshal(data, &aux); err != nil {
@@ -43,15 +44,10 @@ func (i *IdentityStub) UnmarshalJSON(data []byte) error {
 }
 
 // Id return the Identity identifier
-func (i *IdentityStub) Id() string {
+func (i *IdentityStub) Id() entity.Id {
 	return i.id
 }
 
-// HumanId return the Identity identifier truncated for human consumption
-func (i *IdentityStub) HumanId() string {
-	return FormatHumanID(i.Id())
-}
-
 func (IdentityStub) Name() string {
 	panic("identities needs to be properly loaded with identity.ReadLocal()")
 }
diff --git a/identity/identity_stub_test.go b/identity/identity_stub_test.go
index 3d94cd3e..b01a718c 100644
--- a/identity/identity_stub_test.go
+++ b/identity/identity_stub_test.go
@@ -19,5 +19,8 @@ func TestIdentityStubSerialize(t *testing.T) {
 	err = json.Unmarshal(data, &after)
 	assert.NoError(t, err)
 
+	// enforce creating the Id
+	before.Id()
+
 	assert.Equal(t, before, &after)
 }
diff --git a/identity/identity_test.go b/identity/identity_test.go
index 2ddb64ea..f91c548f 100644
--- a/identity/identity_test.go
+++ b/identity/identity_test.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"testing"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/stretchr/testify/assert"
 )
@@ -15,6 +16,7 @@ func TestIdentityCommitLoad(t *testing.T) {
 	// single version
 
 	identity := &Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				name:  "René Descartes",
@@ -36,6 +38,7 @@ func TestIdentityCommitLoad(t *testing.T) {
 	// multiple version
 
 	identity = &Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				time:  100,
@@ -114,6 +117,7 @@ func commitsAreSet(t *testing.T, identity *Identity) {
 // Test that the correct crypto keys are returned for a given lamport time
 func TestIdentity_ValidKeysAtTime(t *testing.T) {
 	identity := Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				time:  100,
@@ -215,6 +219,7 @@ func TestJSON(t *testing.T) {
 	mockRepo := repository.NewMockRepoForTest()
 
 	identity := &Identity{
+		id: entity.UnsetId,
 		versions: []*Version{
 			{
 				name:  "René Descartes",
@@ -223,7 +228,7 @@ func TestJSON(t *testing.T) {
 		},
 	}
 
-	// commit to make sure we have an ID
+	// commit to make sure we have an Id
 	err := identity.Commit(mockRepo)
 	assert.Nil(t, err)
 	assert.NotEmpty(t, identity.id)
diff --git a/identity/interface.go b/identity/interface.go
index 88f1d9a7..54a9da78 100644
--- a/identity/interface.go
+++ b/identity/interface.go
@@ -1,6 +1,7 @@
 package identity
 
 import (
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/lamport"
 	"github.com/MichaelMure/git-bug/util/timestamp"
@@ -8,10 +9,7 @@ import (
 
 type Interface interface {
 	// Id return the Identity identifier
-	Id() string
-
-	// HumanId return the Identity identifier truncated for human consumption
-	HumanId() string
+	Id() entity.Id
 
 	// Name return the last version of the name
 	Name() string
diff --git a/identity/resolver.go b/identity/resolver.go
index 7facfc0c..70fac74c 100644
--- a/identity/resolver.go
+++ b/identity/resolver.go
@@ -1,11 +1,14 @@
 package identity
 
-import "github.com/MichaelMure/git-bug/repository"
+import (
+	"github.com/MichaelMure/git-bug/entity"
+	"github.com/MichaelMure/git-bug/repository"
+)
 
 // Resolver define the interface of an Identity resolver, able to load
 // an identity from, for example, a repo or a cache.
 type Resolver interface {
-	ResolveIdentity(id string) (Interface, error)
+	ResolveIdentity(id entity.Id) (Interface, error)
 }
 
 // DefaultResolver is a Resolver loading Identities directly from a Repo
@@ -17,6 +20,6 @@ func NewSimpleResolver(repo repository.Repo) *SimpleResolver {
 	return &SimpleResolver{repo: repo}
 }
 
-func (r *SimpleResolver) ResolveIdentity(id string) (Interface, error) {
+func (r *SimpleResolver) ResolveIdentity(id entity.Id) (Interface, error) {
 	return ReadLocal(r.repo, id)
 }