entity: more progress on merging and signing

Michael Muré created

Change summary

bug/bug_actions.go           |   6 
cache/repo_cache_bug.go      |   5 
entity/TODO                  |   1 
entity/dag/entity.go         |  14 --
entity/dag/entity_actions.go |  87 +++++++++++++------
entity/dag/operation.go      |   8 +
entity/dag/operation_pack.go |  50 ++++++++++
entity/merge.go              |  37 +++++--
identity/identity.go         |  15 ++
identity/identity_actions.go |   6 
identity/identity_stub.go    |   3 
identity/interface.go        |   3 
identity/key.go              | 163 ++++++++++++++++++++++---------------
identity/key_test.go         |  45 +++++++++
repository/common.go         |  53 ------------
repository/gogit.go          |   7 -
repository/keyring.go        |   2 
repository/repo.go           |   5 -
repository/repo_testing.go   |  21 ----
19 files changed, 303 insertions(+), 228 deletions(-)

Detailed changes

bug/bug_actions.go 🔗

@@ -112,7 +112,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes
 					return
 				}
 
-				out <- entity.NewMergeStatus(entity.MergeStatusNew, id, remoteBug)
+				out <- entity.NewMergeNewStatus(id, remoteBug)
 				continue
 			}
 
@@ -131,9 +131,9 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes
 			}
 
 			if updated {
-				out <- entity.NewMergeStatus(entity.MergeStatusUpdated, id, localBug)
+				out <- entity.NewMergeUpdatedStatus(id, localBug)
 			} else {
-				out <- entity.NewMergeStatus(entity.MergeStatusNothing, id, localBug)
+				out <- entity.NewMergeNothingStatus(id)
 			}
 		}
 	}()

cache/repo_cache_bug.go 🔗

@@ -16,10 +16,7 @@ import (
 	"github.com/blevesearch/bleve"
 )
 
-const (
-	bugCacheFile   = "bug-cache"
-	searchCacheDir = "search-cache"
-)
+const bugCacheFile = "bug-cache"
 
 var errBugNotInCache = errors.New("bug missing from cache")
 

entity/TODO 🔗

@@ -1,6 +1,7 @@
 - is the pack Lamport clock really useful vs only topological sort?
   - topological order is enforced on the clocks, so what's the point?
   - is EditTime equivalent to PackTime? no, avoid the gaps. Is it better?
+    --> PackTime is contained within a bug and might avoid extreme reordering?
 - how to do commit signature?
 - how to avoid id collision between Operations?
 - write tests for actions

entity/dag/entity.go 🔗

@@ -318,7 +318,6 @@ func (e *Entity) CommitAdNeeded(repo repository.ClockedRepo) error {
 }
 
 // Commit write the appended operations in the repository
-// TODO: support commit signature
 func (e *Entity) Commit(repo repository.ClockedRepo) error {
 	if !e.NeedCommit() {
 		return fmt.Errorf("can't commit an entity with no pending operation")
@@ -361,18 +360,13 @@ func (e *Entity) Commit(repo repository.ClockedRepo) error {
 		// PackTime:   packTime,
 	}
 
-	treeHash, err := opp.Write(e.Definition, repo)
-	if err != nil {
-		return err
-	}
-
-	// Write a Git commit referencing the tree, with the previous commit as parent
 	var commitHash repository.Hash
-	if e.lastCommit != "" {
-		commitHash, err = repo.StoreCommit(treeHash, e.lastCommit)
+	if e.lastCommit == "" {
+		commitHash, err = opp.Write(e.Definition, repo)
 	} else {
-		commitHash, err = repo.StoreCommit(treeHash)
+		commitHash, err = opp.Write(e.Definition, repo, e.lastCommit)
 	}
+
 	if err != nil {
 		return err
 	}

entity/dag/entity_actions.go 🔗

@@ -9,6 +9,7 @@ import (
 	"github.com/MichaelMure/git-bug/repository"
 )
 
+// ListLocalIds list all the available local Entity's Id
 func ListLocalIds(typename string, repo repository.RepoData) ([]entity.Id, error) {
 	refs, err := repo.ListRefs(fmt.Sprintf("refs/%s/", typename))
 	if err != nil {
@@ -56,6 +57,21 @@ func Pull(def Definition, repo repository.ClockedRepo, remote string) error {
 	return nil
 }
 
+// MergeAll will merge all the available remote Entity:
+//
+// Multiple scenario exist:
+// 1. if the remote Entity doesn't exist locally, it's created
+//    --> emit entity.MergeStatusNew
+// 2. if the remote and local Entity have the same state, nothing is changed
+//    --> emit entity.MergeStatusNothing
+// 3. if the local Entity has new commits but the remote don't, nothing is changed
+//    --> emit entity.MergeStatusNothing
+// 4. if the remote has new commit, the local bug is updated to match the same history
+//    (fast-forward update)
+//    --> emit entity.MergeStatusUpdated
+// 5. if both local and remote Entity have new commits (that is, we have a concurrent edition),
+//    a merge commit with an empty operationPack is created to join both branch and form a DAG.
+//    --> emit entity.MergeStatusUpdated
 func MergeAll(def Definition, repo repository.ClockedRepo, remote string) <-chan entity.MergeResult {
 	out := make(chan entity.MergeResult)
 
@@ -81,6 +97,8 @@ func MergeAll(def Definition, repo repository.ClockedRepo, remote string) <-chan
 	return out
 }
 
+// merge perform a merge to make sure a local Entity is up to date.
+// See MergeAll for more details.
 func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity.MergeResult {
 	id := entity.RefToId(remoteRef)
 
@@ -102,36 +120,24 @@ func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity
 
 	localRef := fmt.Sprintf("refs/%s/%s", def.namespace, id.String())
 
+	// SCENARIO 1
+	// if the remote Entity doesn't exist locally, it's created
+
 	localExist, err := repo.RefExist(localRef)
 	if err != nil {
 		return entity.NewMergeError(err, id)
 	}
 
-	// the bug is not local yet, simply create the reference
 	if !localExist {
+		// the bug is not local yet, simply create the reference
 		err := repo.CopyRef(remoteRef, localRef)
 		if err != nil {
 			return entity.NewMergeError(err, id)
 		}
 
-		return entity.NewMergeStatus(entity.MergeStatusNew, id, remoteEntity)
+		return entity.NewMergeNewStatus(id, remoteEntity)
 	}
 
-	// var updated bool
-	// err = repo.MergeRef(localRef, remoteRef, func() repository.Hash {
-	// 	updated = true
-	//
-	// })
-	// if err != nil {
-	// 	return entity.NewMergeError(err, id)
-	// }
-	//
-	// if updated {
-	// 	return entity.NewMergeStatus(entity.MergeStatusUpdated, id, )
-	// } else {
-	// 	return entity.NewMergeStatus(entity.MergeStatusNothing, id, )
-	// }
-
 	localCommit, err := repo.ResolveRef(localRef)
 	if err != nil {
 		return entity.NewMergeError(err, id)
@@ -142,18 +148,38 @@ func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity
 		return entity.NewMergeError(err, id)
 	}
 
+	// SCENARIO 2
+	// if the remote and local Entity have the same state, nothing is changed
+
 	if localCommit == remoteCommit {
 		// nothing to merge
-		return entity.NewMergeStatus(entity.MergeStatusNothing, id, remoteEntity)
+		return entity.NewMergeNothingStatus(id)
 	}
 
-	// fast-forward is possible if otherRef include ref
+	// SCENARIO 3
+	// if the local Entity has new commits but the remote don't, nothing is changed
+
+	localCommits, err := repo.ListCommits(localRef)
+	if err != nil {
+		return entity.NewMergeError(err, id)
+	}
+
+	for _, hash := range localCommits {
+		if hash == localCommit {
+			return entity.NewMergeNothingStatus(id)
+		}
+	}
+
+	// SCENARIO 4
+	// if the remote has new commit, the local bug is updated to match the same history
+	// (fast-forward update)
 
 	remoteCommits, err := repo.ListCommits(remoteRef)
 	if err != nil {
 		return entity.NewMergeError(err, id)
 	}
 
+	// fast-forward is possible if otherRef include ref
 	fastForwardPossible := false
 	for _, hash := range remoteCommits {
 		if hash == localCommit {
@@ -167,9 +193,13 @@ func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity
 		if err != nil {
 			return entity.NewMergeError(err, id)
 		}
-		return entity.NewMergeStatus(entity.MergeStatusUpdated, id, remoteEntity)
+		return entity.NewMergeUpdatedStatus(id, remoteEntity)
 	}
 
+	// SCENARIO 5
+	// if both local and remote Entity have new commits (that is, we have a concurrent edition),
+	// a merge commit with an empty operationPack is created to join both branch and form a DAG.
+
 	// fast-forward is not possible, we need to create a merge commit
 	// For simplicity when reading and to have clocks that record this change, we store
 	// an empty operationPack.
@@ -180,6 +210,7 @@ func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity
 		return entity.NewMergeError(err, id)
 	}
 
+	// TODO: pack clock
 	// err = localEntity.packClock.Witness(remoteEntity.packClock.Time())
 	// if err != nil {
 	// 	return entity.NewMergeError(err, id)
@@ -199,27 +230,25 @@ func merge(def Definition, repo repository.ClockedRepo, remoteRef string) entity
 		Operations: nil,
 		CreateTime: 0,
 		EditTime:   editTime,
+		// TODO: pack clock
 		// PackTime:   packTime,
 	}
 
-	treeHash, err := opp.Write(def, repo)
-	if err != nil {
-		return entity.NewMergeError(err, id)
-	}
-
-	// Create the merge commit with two parents
-	newHash, err := repo.StoreCommit(treeHash, localCommit, remoteCommit)
+	commitHash, err := opp.Write(def, repo, localCommit, remoteCommit)
 	if err != nil {
 		return entity.NewMergeError(err, id)
 	}
 
 	// finally update the ref
-	err = repo.UpdateRef(localRef, newHash)
+	err = repo.UpdateRef(localRef, commitHash)
 	if err != nil {
 		return entity.NewMergeError(err, id)
 	}
 
-	return entity.NewMergeStatus(entity.MergeStatusUpdated, id, localEntity)
+	// Note: we don't need to update localEntity state (lastCommit, operations...) as we
+	// discard it entirely anyway.
+
+	return entity.NewMergeUpdatedStatus(id, localEntity)
 }
 
 func Remove() error {

entity/dag/operation.go 🔗

@@ -12,17 +12,19 @@ type Operation interface {
 	// Id return the Operation identifier
 	// Some care need to be taken to define a correct Id derivation and enough entropy in the data used to avoid
 	// collisions. Notably:
-	// - the Id of the first Operation will be used as the Id of the Entity. Collision need to be avoided across Entities.
+	// - the Id of the first Operation will be used as the Id of the Entity. Collision need to be avoided across entities of the same type
+	//   (example: no collision within the "bug" namespace).
 	// - collisions can also happen within the set of Operations of an Entity. Simple Operation might not have enough
-	//   entropy to yield unique Ids.
+	//   entropy to yield unique Ids (example: two "close" operation within the same second, same author).
 	// A common way to derive an Id will be to use the DeriveId function on the serialized operation data.
 	Id() entity.Id
 	// Validate check if the Operation data is valid
 	Validate() error
-
+	// Author returns the author of this operation
 	Author() identity.Interface
 }
 
+// TODO: remove?
 type operationBase struct {
 	author identity.Interface
 

entity/dag/operation_pack.go 🔗

@@ -86,7 +86,10 @@ func (opp *operationPack) Validate() error {
 	return nil
 }
 
-func (opp *operationPack) Write(def Definition, repo repository.RepoData, parentCommit ...repository.Hash) (repository.Hash, error) {
+// Write write the OperationPack in git, with zero, one or more parent commits.
+// If the repository has a keypair able to sign (that is, with a private key), the resulting commit is signed with that key.
+// Return the hash of the created commit.
+func (opp *operationPack) Write(def Definition, repo repository.Repo, parentCommit ...repository.Hash) (repository.Hash, error) {
 	if err := opp.Validate(); err != nil {
 		return "", err
 	}
@@ -148,8 +151,13 @@ func (opp *operationPack) Write(def Definition, repo repository.RepoData, parent
 	var commitHash repository.Hash
 
 	// Sign the commit if we have a key
-	if opp.Author.SigningKey() != nil {
-		commitHash, err = repo.StoreSignedCommit(treeHash, opp.Author.SigningKey().PGPEntity(), parentCommit...)
+	signingKey, err := opp.Author.SigningKey(repo)
+	if err != nil {
+		return "", err
+	}
+
+	if signingKey != nil {
+		commitHash, err = repo.StoreSignedCommit(treeHash, signingKey.PGPEntity(), parentCommit...)
 	} else {
 		commitHash, err = repo.StoreCommit(treeHash, parentCommit...)
 	}
@@ -240,7 +248,7 @@ func readOperationPack(def Definition, repo repository.RepoData, commit reposito
 	// Verify signature if we expect one
 	keys := author.ValidKeysAtTime(fmt.Sprintf(editClockPattern, def.namespace), editTime)
 	if len(keys) > 0 {
-		keyring := identity.PGPKeyring(keys)
+		keyring := PGPKeyring(keys)
 		_, err = openpgp.CheckDetachedSignature(keyring, commit.SignedData, commit.Signature)
 		if err != nil {
 			return nil, fmt.Errorf("signature failure: %v", err)
@@ -292,3 +300,37 @@ func unmarshallPack(def Definition, data []byte) ([]Operation, identity.Interfac
 
 	return ops, author, nil
 }
+
+var _ openpgp.KeyRing = &PGPKeyring{}
+
+// PGPKeyring implement a openpgp.KeyRing from an slice of Key
+type PGPKeyring []*identity.Key
+
+func (pk PGPKeyring) KeysById(id uint64) []openpgp.Key {
+	var result []openpgp.Key
+	for _, key := range pk {
+		if key.Public().KeyId == id {
+			result = append(result, openpgp.Key{
+				PublicKey:  key.Public(),
+				PrivateKey: key.Private(),
+			})
+		}
+	}
+	return result
+}
+
+func (pk PGPKeyring) KeysByIdUsage(id uint64, requiredUsage byte) []openpgp.Key {
+	// the only usage we care about is the ability to sign, which all keys should already be capable of
+	return pk.KeysById(id)
+}
+
+func (pk PGPKeyring) DecryptionKeys() []openpgp.Key {
+	result := make([]openpgp.Key, len(pk))
+	for i, key := range pk {
+		result[i] = openpgp.Key{
+			PublicKey:  key.Public(),
+			PrivateKey: key.Private(),
+		}
+	}
+	return result
+}

entity/merge.go 🔗

@@ -24,10 +24,10 @@ type MergeResult struct {
 	Id     Id
 	Status MergeStatus
 
-	// Only set for invalid status
+	// Only set for Invalid status
 	Reason string
 
-	// Not set for invalid status
+	// Only set for New or Updated status
 	Entity Interface
 }
 
@@ -48,29 +48,42 @@ func (mr MergeResult) String() string {
 	}
 }
 
-func NewMergeError(err error, id Id) MergeResult {
+// TODO: Interface --> *Entity ?
+func NewMergeNewStatus(id Id, entity Interface) MergeResult {
 	return MergeResult{
-		Err:    err,
 		Id:     id,
-		Status: MergeStatusError,
+		Status: MergeStatusNew,
+		Entity: entity,
 	}
 }
 
-// TODO: Interface --> *Entity ?
-func NewMergeStatus(status MergeStatus, id Id, entity Interface) MergeResult {
+func NewMergeInvalidStatus(id Id, reason string) MergeResult {
 	return MergeResult{
 		Id:     id,
-		Status: status,
+		Status: MergeStatusInvalid,
+		Reason: reason,
+	}
+}
 
-		// Entity is not set for an invalid merge result
+func NewMergeUpdatedStatus(id Id, entity Interface) MergeResult {
+	return MergeResult{
+		Id:     id,
+		Status: MergeStatusUpdated,
 		Entity: entity,
 	}
 }
 
-func NewMergeInvalidStatus(id Id, reason string) MergeResult {
+func NewMergeNothingStatus(id Id) MergeResult {
 	return MergeResult{
 		Id:     id,
-		Status: MergeStatusInvalid,
-		Reason: reason,
+		Status: MergeStatusNothing,
+	}
+}
+
+func NewMergeError(err error, id Id) MergeResult {
+	return MergeResult{
+		Id:     id,
+		Status: MergeStatusError,
+		Err:    err,
 	}
 }

identity/identity.go 🔗

@@ -519,12 +519,19 @@ func (i *Identity) Keys() []*Key {
 }
 
 // SigningKey return the key that should be used to sign new messages. If no key is available, return nil.
-func (i *Identity) SigningKey() *Key {
+func (i *Identity) SigningKey(repo repository.RepoKeyring) (*Key, error) {
 	keys := i.Keys()
-	if len(keys) > 0 {
-		return keys[0]
+	for _, key := range keys {
+		err := key.ensurePrivateKey(repo)
+		if err == errNoPrivateKey {
+			continue
+		}
+		if err != nil {
+			return nil, err
+		}
+		return key, nil
 	}
-	return nil
+	return nil, nil
 }
 
 // ValidKeysAtTime return the set of keys valid at a given lamport time

identity/identity_actions.go 🔗

@@ -102,7 +102,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes
 					return
 				}
 
-				out <- entity.NewMergeStatus(entity.MergeStatusNew, id, remoteIdentity)
+				out <- entity.NewMergeNewStatus(id, remoteIdentity)
 				continue
 			}
 
@@ -121,9 +121,9 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes
 			}
 
 			if updated {
-				out <- entity.NewMergeStatus(entity.MergeStatusUpdated, id, localIdentity)
+				out <- entity.NewMergeUpdatedStatus(id, localIdentity)
 			} else {
-				out <- entity.NewMergeStatus(entity.MergeStatusNothing, id, localIdentity)
+				out <- entity.NewMergeNothingStatus(id)
 			}
 		}
 	}()

identity/identity_stub.go 🔗

@@ -4,6 +4,7 @@ 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"
 )
@@ -71,7 +72,7 @@ func (IdentityStub) Keys() []*Key {
 	panic("identities needs to be properly loaded with identity.ReadLocal()")
 }
 
-func (i *IdentityStub) SigningKey() *Key {
+func (i *IdentityStub) SigningKey(repo repository.RepoKeyring) (*Key, error) {
 	panic("identities needs to be properly loaded with identity.ReadLocal()")
 }
 

identity/interface.go 🔗

@@ -2,6 +2,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"
 )
@@ -37,7 +38,7 @@ type Interface interface {
 	Keys() []*Key
 
 	// SigningKey return the key that should be used to sign new messages. If no key is available, return nil.
-	SigningKey() *Key
+	SigningKey(repo repository.RepoKeyring) (*Key, error)
 
 	// ValidKeysAtTime return the set of keys valid at a given lamport time for a given clock of another entity
 	// Can be empty.

identity/key.go 🔗

@@ -16,12 +16,15 @@ import (
 	"github.com/MichaelMure/git-bug/repository"
 )
 
+var errNoPrivateKey = fmt.Errorf("no private key")
+
 type Key struct {
 	public  *packet.PublicKey
 	private *packet.PrivateKey
 }
 
 // GenerateKey generate a keypair (public+private)
+// The type and configuration of the key is determined by the default value in go's OpenPGP.
 func GenerateKey() *Key {
 	entity, err := openpgp.NewEntity("", "", "", &packet.Config{
 		// The armored format doesn't include the creation time, which makes the round-trip data not being fully equal.
@@ -47,12 +50,53 @@ func generatePublicKey() *Key {
 	return k
 }
 
+func (k *Key) Public() *packet.PublicKey {
+	return k.public
+}
+
+func (k *Key) Private() *packet.PrivateKey {
+	return k.private
+}
+
+func (k *Key) Validate() error {
+	if k.public == nil {
+		return fmt.Errorf("nil public key")
+	}
+	if !k.public.CanSign() {
+		return fmt.Errorf("public key can't sign")
+	}
+
+	if k.private != nil {
+		if !k.private.CanSign() {
+			return fmt.Errorf("private key can't sign")
+		}
+	}
+
+	return nil
+}
+
+func (k *Key) Clone() *Key {
+	clone := &Key{}
+
+	pub := *k.public
+	clone.public = &pub
+
+	if k.private != nil {
+		priv := *k.private
+		clone.private = &priv
+	}
+
+	return clone
+}
+
 func (k *Key) MarshalJSON() ([]byte, error) {
+	// Serialize only the public key, in the armored format.
 	var buf bytes.Buffer
 	w, err := armor.Encode(&buf, openpgp.PublicKeyType, nil)
 	if err != nil {
 		return nil, err
 	}
+
 	err = k.public.Serialize(w)
 	if err != nil {
 		return nil, err
@@ -65,6 +109,7 @@ func (k *Key) MarshalJSON() ([]byte, error) {
 }
 
 func (k *Key) UnmarshalJSON(data []byte) error {
+	// De-serialize only the public key, in the armored format.
 	var armored string
 	err := json.Unmarshal(data, &armored)
 	if err != nil {
@@ -83,8 +128,7 @@ func (k *Key) UnmarshalJSON(data []byte) error {
 		return fmt.Errorf("invalid key type")
 	}
 
-	reader := packet.NewReader(block.Body)
-	p, err := reader.Next()
+	p, err := packet.Read(block.Body)
 	if err != nil {
 		return errors.Wrap(err, "failed to read public key packet")
 	}
@@ -102,53 +146,74 @@ func (k *Key) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
-func (k *Key) Validate() error {
-	if k.public == nil {
-		return fmt.Errorf("nil public key")
+func (k *Key) loadPrivate(repo repository.RepoKeyring) error {
+	item, err := repo.Keyring().Get(k.public.KeyIdString())
+	if err == repository.ErrKeyringKeyNotFound {
+		return errNoPrivateKey
 	}
-	if !k.public.CanSign() {
-		return fmt.Errorf("public key can't sign")
+	if err != nil {
+		return err
 	}
 
-	if k.private != nil {
-		if !k.private.CanSign() {
-			return fmt.Errorf("private key can't sign")
-		}
+	block, err := armor.Decode(bytes.NewReader(item.Data))
+	if err == io.EOF {
+		return fmt.Errorf("no armored data found")
+	}
+	if err != nil {
+		return err
 	}
 
-	return nil
-}
-
-func (k *Key) Clone() *Key {
-	clone := &Key{}
+	if block.Type != openpgp.PrivateKeyType {
+		return fmt.Errorf("invalid key type")
+	}
 
-	pub := *k.public
-	clone.public = &pub
+	p, err := packet.Read(block.Body)
+	if err != nil {
+		return errors.Wrap(err, "failed to read private key packet")
+	}
 
-	if k.private != nil {
-		priv := *k.private
-		clone.private = &priv
+	private, ok := p.(*packet.PrivateKey)
+	if !ok {
+		return errors.New("got no packet.privateKey")
 	}
 
-	return clone
+	// The armored format doesn't include the creation time, which makes the round-trip data not being fully equal.
+	// We don't care about the creation time so we can set it to the zero value.
+	private.CreationTime = time.Time{}
+
+	k.private = private
+	return nil
 }
 
-func (k *Key) EnsurePrivateKey(repo repository.RepoKeyring) error {
+// ensurePrivateKey attempt to load the corresponding private key if it is not loaded already.
+// If no private key is found, returns errNoPrivateKey
+func (k *Key) ensurePrivateKey(repo repository.RepoKeyring) error {
 	if k.private != nil {
 		return nil
 	}
 
-	// item, err := repo.Keyring().Get(k.Fingerprint())
-	// if err != nil {
-	// 	return fmt.Errorf("no private key found for %s", k.Fingerprint())
-	// }
-	//
-
-	panic("TODO")
+	return k.loadPrivate(repo)
 }
 
-func (k *Key) Fingerprint() string {
-	return string(k.public.Fingerprint[:])
+func (k *Key) storePrivate(repo repository.RepoKeyring) error {
+	var buf bytes.Buffer
+	w, err := armor.Encode(&buf, openpgp.PrivateKeyType, nil)
+	if err != nil {
+		return err
+	}
+	err = k.private.Serialize(w)
+	if err != nil {
+		return err
+	}
+	err = w.Close()
+	if err != nil {
+		return err
+	}
+
+	return repo.Keyring().Set(repository.Item{
+		Key:  k.public.KeyIdString(),
+		Data: buf.Bytes(),
+	})
 }
 
 func (k *Key) PGPEntity() *openpgp.Entity {
@@ -157,37 +222,3 @@ func (k *Key) PGPEntity() *openpgp.Entity {
 		PrivateKey: k.private,
 	}
 }
-
-var _ openpgp.KeyRing = &PGPKeyring{}
-
-// PGPKeyring implement a openpgp.KeyRing from an slice of Key
-type PGPKeyring []*Key
-
-func (pk PGPKeyring) KeysById(id uint64) []openpgp.Key {
-	var result []openpgp.Key
-	for _, key := range pk {
-		if key.public.KeyId == id {
-			result = append(result, openpgp.Key{
-				PublicKey:  key.public,
-				PrivateKey: key.private,
-			})
-		}
-	}
-	return result
-}
-
-func (pk PGPKeyring) KeysByIdUsage(id uint64, requiredUsage byte) []openpgp.Key {
-	// the only usage we care about is the ability to sign, which all keys should already be capable of
-	return pk.KeysById(id)
-}
-
-func (pk PGPKeyring) DecryptionKeys() []openpgp.Key {
-	result := make([]openpgp.Key, len(pk))
-	for i, key := range pk {
-		result[i] = openpgp.Key{
-			PublicKey:  key.public,
-			PrivateKey: key.private,
-		}
-	}
-	return result
-}

identity/key_test.go 🔗

@@ -1,21 +1,60 @@
 package identity
 
 import (
+	"crypto/rsa"
 	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/MichaelMure/git-bug/repository"
 )
 
-func TestKeyJSON(t *testing.T) {
+func TestPublicKeyJSON(t *testing.T) {
 	k := generatePublicKey()
 
-	data, err := json.Marshal(k)
+	dataJSON, err := json.Marshal(k)
 	require.NoError(t, err)
 
 	var read Key
-	err = json.Unmarshal(data, &read)
+	err = json.Unmarshal(dataJSON, &read)
 	require.NoError(t, err)
 
 	require.Equal(t, k, &read)
 }
+
+func TestStoreLoad(t *testing.T) {
+	repo := repository.NewMockRepoKeyring()
+
+	// public + private
+	k := GenerateKey()
+
+	// Store
+
+	dataJSON, err := json.Marshal(k)
+	require.NoError(t, err)
+
+	err = k.storePrivate(repo)
+	require.NoError(t, err)
+
+	// Load
+
+	var read Key
+	err = json.Unmarshal(dataJSON, &read)
+	require.NoError(t, err)
+
+	err = read.ensurePrivateKey(repo)
+	require.NoError(t, err)
+
+	require.Equal(t, k.public, read.public)
+
+	require.IsType(t, (*rsa.PrivateKey)(nil), k.private.PrivateKey)
+
+	// See https://github.com/golang/crypto/pull/175
+	rsaPriv := read.private.PrivateKey.(*rsa.PrivateKey)
+	back := rsaPriv.Primes[0]
+	rsaPriv.Primes[0] = rsaPriv.Primes[1]
+	rsaPriv.Primes[1] = back
+
+	require.True(t, k.private.PrivateKey.(*rsa.PrivateKey).Equal(read.private.PrivateKey))
+}

repository/common.go 🔗

@@ -8,59 +8,6 @@ import (
 	"golang.org/x/crypto/openpgp/errors"
 )
 
-// nonNativeMerge is an implementation of a branch merge, for the case where
-// the underlying git implementation doesn't support it natively.
-func nonNativeMerge(repo RepoData, ref string, otherRef string, treeHashFn func() Hash) error {
-	commit, err := repo.ResolveRef(ref)
-	if err != nil {
-		return err
-	}
-
-	otherCommit, err := repo.ResolveRef(otherRef)
-	if err != nil {
-		return err
-	}
-
-	if commit == otherCommit {
-		// nothing to merge
-		return nil
-	}
-
-	// fast-forward is possible if otherRef include ref
-
-	otherCommits, err := repo.ListCommits(otherRef)
-	if err != nil {
-		return err
-	}
-
-	fastForwardPossible := false
-	for _, hash := range otherCommits {
-		if hash == commit {
-			fastForwardPossible = true
-			break
-		}
-	}
-
-	if fastForwardPossible {
-		return repo.UpdateRef(ref, otherCommit)
-	}
-
-	// fast-forward is not possible, we need to create a merge commit
-
-	// we need a Tree to make the commit, an empty Tree will do
-	emptyTreeHash, err := repo.StoreTree(nil)
-	if err != nil {
-		return err
-	}
-
-	newHash, err := repo.StoreCommit(emptyTreeHash, commit, otherCommit)
-	if err != nil {
-		return err
-	}
-
-	return repo.UpdateRef(ref, newHash)
-}
-
 // nonNativeListCommits is an implementation for ListCommits, for the case where
 // the underlying git implementation doesn't support if natively.
 func nonNativeListCommits(repo RepoData, ref string) ([]Hash, error) {

repository/gogit.go 🔗

@@ -630,13 +630,6 @@ func (repo *GoGitRepo) UpdateRef(ref string, hash Hash) error {
 	return repo.r.Storer.SetReference(plumbing.NewHashReference(plumbing.ReferenceName(ref), plumbing.NewHash(hash.String())))
 }
 
-// MergeRef merge other into ref and update the reference
-// If the update is not fast-forward, the callback treeHashFn will be called for the caller to generate
-// the Tree to store in the merge commit.
-func (repo *GoGitRepo) MergeRef(ref string, otherRef string, treeHashFn func() Hash) error {
-	return nonNativeMerge(repo, ref, otherRef, treeHashFn)
-}
-
 // RemoveRef will remove a Git reference
 func (repo *GoGitRepo) RemoveRef(ref string) error {
 	return repo.r.Storer.RemoveReference(plumbing.ReferenceName(ref))

repository/keyring.go 🔗

@@ -15,7 +15,7 @@ var ErrKeyringKeyNotFound = keyring.ErrKeyNotFound
 type Keyring interface {
 	// Returns an Item matching the key or ErrKeyringKeyNotFound
 	Get(key string) (Item, error)
-	// Stores an Item on the keyring
+	// Stores an Item on the keyring. Set is idempotent.
 	Set(item Item) error
 	// Removes the item with matching key
 	Remove(key string) error

repository/repo.go 🔗

@@ -138,11 +138,6 @@ type RepoData interface {
 	// UpdateRef will create or update a Git reference
 	UpdateRef(ref string, hash Hash) error
 
-	// // MergeRef merge other into ref and update the reference
-	// // If the update is not fast-forward, the callback treeHashFn will be called for the caller to generate
-	// // the Tree to store in the merge commit.
-	// MergeRef(ref string, otherRef string, treeHashFn func() Hash) error
-
 	// RemoveRef will remove a Git reference
 	RemoveRef(ref string) error
 

repository/repo_testing.go 🔗

@@ -197,6 +197,8 @@ func RepoDataTest(t *testing.T, repo RepoData) {
 
 	err = repo.RemoveRef("refs/bugs/ref1")
 	require.NoError(t, err)
+
+	// TODO: testing for commit's signature
 }
 
 // helper to test a RepoClock
@@ -238,22 +240,3 @@ func randomData() []byte {
 	}
 	return b
 }
-
-func makeCommit(t *testing.T, repo RepoData, parents ...Hash) Hash {
-	blobHash, err := repo.StoreData(randomData())
-	require.NoError(t, err)
-
-	treeHash, err := repo.StoreTree([]TreeEntry{
-		{
-			ObjectType: Blob,
-			Hash:       blobHash,
-			Name:       "foo",
-		},
-	})
-	require.NoError(t, err)
-
-	commitHash, err := repo.StoreCommit(treeHash, parents...)
-	require.NoError(t, err)
-
-	return commitHash
-}