Merge pull request #8 from MichaelMure/update-migration-3

update migration 3, in particular for git-bug/git-bug/pull/767

Author: MichaelMure

go.mod

@@ -4,6 +4,7 @@ go 1.14
 
 require (
 	github.com/99designs/keyring v1.1.5
+	github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7
 	github.com/blang/semver v3.5.1+incompatible
 	github.com/blevesearch/bleve v1.0.14
 	github.com/dustin/go-humanize v1.0.0
@@ -14,7 +15,9 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/spf13/cobra v1.0.0
 	github.com/stretchr/testify v1.6.1
-	golang.org/x/crypto v0.0.0-20200302210943-78000ba7a073
+	golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2
 	golang.org/x/sys v0.0.0-20210403161142-5e06dd20ab57
 	golang.org/x/text v0.3.3
 )
+
+replace golang.org/x/crypto => golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2

go.sum

@@ -28,7 +28,7 @@ var def = dag.Definition{
 
 var ClockLoader = dag.ClockLoader(def)
 
-// Bug hold the data of a bug thread, organized in a way close to
+// Bug holds the data of a bug thread, organized in a way close to
 // how it will be persisted inside Git. This is the data structure
 // used to merge two different version of the same Bug.
 type Bug struct {

migration3/after/bug/bug.go

@@ -22,13 +22,15 @@ func Push(repo repository.Repo, remote string) (string, error) {
 
 // Pull will do a Fetch + MergeAll
 // This function will return an error if a merge fail
-func Pull(repo repository.ClockedRepo, remote string, author identity.Interface) error {
+// Note: an author is necessary for the case where a merge commit is created, as this commit will
+// have an author and may be signed if a signing key is available.
+func Pull(repo repository.ClockedRepo, remote string, mergeAuthor identity.Interface) error {
 	_, err := Fetch(repo, remote)
 	if err != nil {
 		return err
 	}
 
-	for merge := range MergeAll(repo, remote, author) {
+	for merge := range MergeAll(repo, remote, mergeAuthor) {
 		if merge.Err != nil {
 			return merge.Err
 		}
@@ -43,7 +45,7 @@ func Pull(repo repository.ClockedRepo, remote string, author identity.Interface)
 // MergeAll will merge all the available remote bug
 // Note: an author is necessary for the case where a merge commit is created, as this commit will
 // have an author and may be signed if a signing key is available.
-func MergeAll(repo repository.ClockedRepo, remote string, author identity.Interface) <-chan entity.MergeResult {
+func MergeAll(repo repository.ClockedRepo, remote string, mergeAuthor identity.Interface) <-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.
@@ -54,7 +56,7 @@ func MergeAll(repo repository.ClockedRepo, remote string, author identity.Interf
 	go func() {
 		defer close(out)
 
-		results := dag.MergeAll(def, repo, identityResolver, remote, author)
+		results := dag.MergeAll(def, repo, identityResolver, remote, mergeAuthor)
 
 		// wrap the dag.Entity into a complete Bug
 		for result := range results {

migration3/after/bug/bug_actions.go

@@ -31,9 +31,8 @@ func (op *AddCommentOperation) Apply(snapshot *Snapshot) {
 	snapshot.addActor(op.Author_)
 	snapshot.addParticipant(op.Author_)
 
-	commentId := entity.CombineIds(snapshot.Id(), op.Id())
 	comment := Comment{
-		id:       commentId,
+		id:       entity.CombineIds(snapshot.Id(), op.Id()),
 		Message:  op.Message,
 		Author:   op.Author_,
 		Files:    op.Files,
@@ -43,7 +42,7 @@ func (op *AddCommentOperation) Apply(snapshot *Snapshot) {
 	snapshot.Comments = append(snapshot.Comments, comment)
 
 	item := &AddCommentTimelineItem{
-		CommentTimelineItem: NewCommentTimelineItem(commentId, comment),
+		CommentTimelineItem: NewCommentTimelineItem(comment),
 	}
 
 	snapshot.Timeline = append(snapshot.Timeline, item)
@@ -65,7 +64,7 @@ func (op *AddCommentOperation) Validate() error {
 	return nil
 }
 
-// UnmarshalJSON is a two step JSON unmarshalling
+// UnmarshalJSON is a two-steps JSON unmarshalling
 // This workaround is necessary to avoid the inner OpBase.MarshalJSON
 // overriding the outer op's MarshalJSON
 func (op *AddCommentOperation) UnmarshalJSON(data []byte) error {

migration3/after/bug/op_add_comment.go

@@ -3,7 +3,6 @@ package bug
 import (
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"github.com/MichaelMure/git-bug-migration/migration3/after/entity"
 	"github.com/MichaelMure/git-bug-migration/migration3/after/entity/dag"
@@ -56,9 +55,8 @@ func (op *CreateOperation) Apply(snapshot *Snapshot) {
 
 	snapshot.Title = op.Title
 
-	commentId := entity.CombineIds(snapshot.Id(), op.Id())
 	comment := Comment{
-		id:       commentId,
+		id:       entity.CombineIds(snapshot.Id(), op.Id()),
 		Message:  op.Message,
 		Author:   op.Author_,
 		UnixTime: timestamp.Timestamp(op.UnixTime),
@@ -70,7 +68,7 @@ func (op *CreateOperation) Apply(snapshot *Snapshot) {
 
 	snapshot.Timeline = []TimelineItem{
 		&CreateTimelineItem{
-			CommentTimelineItem: NewCommentTimelineItem(commentId, comment),
+			CommentTimelineItem: NewCommentTimelineItem(comment),
 		},
 	}
 }
@@ -94,11 +92,8 @@ func (op *CreateOperation) Validate() error {
 	if text.Empty(op.Title) {
 		return fmt.Errorf("title is empty")
 	}
-	if strings.Contains(op.Title, "\n") {
-		return fmt.Errorf("title should be a single line")
-	}
-	if !text.Safe(op.Title) {
-		return fmt.Errorf("title is not fully printable")
+	if !text.SafeOneLine(op.Title) {
+		return fmt.Errorf("title has unsafe characters")
 	}
 
 	if !text.Safe(op.Message) {

migration3/after/bug/op_create.go

@@ -34,12 +34,12 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 	// Todo: currently any message can be edited, even by a different author
 	// crypto signature are needed.
 
-	snapshot.addActor(op.Author_)
+	// Recreate the Comment Id to match on
+	commentId := entity.CombineIds(snapshot.Id(), op.Target)
 
 	var target TimelineItem
-
 	for i, item := range snapshot.Timeline {
-		if item.Id() == op.Target {
+		if item.Id() == commentId {
 			target = snapshot.Timeline[i]
 			break
 		}
@@ -51,7 +51,7 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 	}
 
 	comment := Comment{
-		id:       op.Target,
+		id:       commentId,
 		Message:  op.Message,
 		Files:    op.Files,
 		UnixTime: timestamp.Timestamp(op.UnixTime),
@@ -62,12 +62,18 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 		target.Append(comment)
 	case *AddCommentTimelineItem:
 		target.Append(comment)
+	default:
+		// somehow, the target matched on something that is not a comment
+		// we make the op a no-op
+		return
 	}
 
+	snapshot.addActor(op.Author_)
+
 	// Updating the corresponding comment
 
 	for i := range snapshot.Comments {
-		if snapshot.Comments[i].Id() == op.Target {
+		if snapshot.Comments[i].Id() == commentId {
 			snapshot.Comments[i].Message = op.Message
 			snapshot.Comments[i].Files = op.Files
 			break
@@ -95,7 +101,7 @@ func (op *EditCommentOperation) Validate() error {
 	return nil
 }
 
-// UnmarshalJSON is a two step JSON unmarshalling
+// UnmarshalJSON is two steps JSON unmarshalling
 // This workaround is necessary to avoid the inner OpBase.MarshalJSON
 // overriding the outer op's MarshalJSON
 func (op *EditCommentOperation) UnmarshalJSON(data []byte) error {
@@ -138,7 +144,7 @@ func NewEditCommentOp(author identity.Interface, unixTime int64, target entity.I
 	}
 }
 
-// Convenience function to apply the operation
+// EditComment is a convenience function to apply the operation
 func EditComment(b Interface, author identity.Interface, unixTime int64, target entity.Id, message string) (*EditCommentOperation, error) {
 	return EditCommentWithFiles(b, author, unixTime, target, message, nil)
 }
@@ -152,13 +158,13 @@ func EditCommentWithFiles(b Interface, author identity.Interface, unixTime int64
 	return editCommentOp, nil
 }
 
-// Convenience function to edit the body of a bug (the first comment)
+// EditCreateComment is a convenience function to edit the body of a bug (the first comment)
 func EditCreateComment(b Interface, author identity.Interface, unixTime int64, message string) (*EditCommentOperation, error) {
 	createOp := b.FirstOp().(*CreateOperation)
 	return EditComment(b, author, unixTime, createOp.Id(), message)
 }
 
-// Convenience function to edit the body of a bug (the first comment)
+// EditCreateCommentWithFiles is a convenience function to edit the body of a bug (the first comment)
 func EditCreateCommentWithFiles(b Interface, author identity.Interface, unixTime int64, message string, files []repository.Hash) (*EditCommentOperation, error) {
 	createOp := b.FirstOp().(*CreateOperation)
 	return EditCommentWithFiles(b, author, unixTime, createOp.Id(), message, files)

migration3/after/bug/op_edit_comment.go

@@ -2,11 +2,13 @@ package bug
 
 import (
 	"encoding/json"
+	"fmt"
 
 	"github.com/pkg/errors"
 
 	"github.com/MichaelMure/git-bug-migration/migration3/after/entity"
 	"github.com/MichaelMure/git-bug-migration/migration3/after/identity"
+	"github.com/MichaelMure/git-bug-migration/migration3/after/util/text"
 )
 
 var _ Operation = &SetMetadataOperation{}
@@ -43,6 +45,15 @@ func (op *SetMetadataOperation) Validate() error {
 		return errors.Wrap(err, "target invalid")
 	}
 
+	for key, val := range op.NewMetadata {
+		if !text.SafeOneLine(key) {
+			return fmt.Errorf("metadata key is unsafe")
+		}
+		if !text.Safe(val) {
+			return fmt.Errorf("metadata value is not fully printable")
+		}
+	}
+
 	return nil
 }
 

migration3/after/bug/op_set_metadata.go

@@ -3,13 +3,11 @@ package bug
 import (
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"github.com/MichaelMure/git-bug-migration/migration3/after/entity"
 	"github.com/MichaelMure/git-bug-migration/migration3/after/identity"
-	"github.com/MichaelMure/git-bug-migration/migration3/after/util/timestamp"
-
 	"github.com/MichaelMure/git-bug-migration/migration3/after/util/text"
+	"github.com/MichaelMure/git-bug-migration/migration3/after/util/timestamp"
 )
 
 var _ Operation = &SetTitleOperation{}
@@ -49,20 +47,12 @@ func (op *SetTitleOperation) Validate() error {
 		return fmt.Errorf("title is empty")
 	}
 
-	if strings.Contains(op.Title, "\n") {
-		return fmt.Errorf("title should be a single line")
-	}
-
-	if !text.Safe(op.Title) {
-		return fmt.Errorf("title should be fully printable")
-	}
-
-	if strings.Contains(op.Was, "\n") {
-		return fmt.Errorf("previous title should be a single line")
+	if !text.SafeOneLine(op.Title) {
+		return fmt.Errorf("title has unsafe characters")
 	}
 
-	if !text.Safe(op.Was) {
-		return fmt.Errorf("previous title should be fully printable")
+	if !text.SafeOneLine(op.Was) {
+		return fmt.Errorf("previous title has unsafe characters")
 	}
 
 	return nil

migration3/after/bug/op_set_title.go

@@ -71,7 +71,7 @@ func idOperation(op Operation, base *OpBase) entity.Id {
 	return base.id
 }
 
-func operationUnmarshaller(author identity.Interface, raw json.RawMessage) (dag.Operation, error) {
+func operationUnmarshaller(author identity.Interface, raw json.RawMessage, resolver identity.Resolver) (dag.Operation, error) {
 	var t struct {
 		OperationType OperationType `json:"type"`
 	}
@@ -135,7 +135,7 @@ func operationUnmarshaller(author identity.Interface, raw json.RawMessage) (dag.
 // OpBase implement the common code for all operations
 type OpBase struct {
 	OperationType OperationType      `json:"type"`
-	Author_       identity.Interface `json:"author"`
+	Author_       identity.Interface `json:"-"` // not serialized
 	// TODO: part of the data model upgrade, this should eventually be a timestamp + lamport
 	UnixTime int64             `json:"timestamp"`
 	Metadata map[string]string `json:"metadata,omitempty"`
@@ -178,7 +178,6 @@ func (base *OpBase) UnmarshalJSON(data []byte) error {
 
 	aux := struct {
 		OperationType OperationType     `json:"type"`
-		Author        json.RawMessage   `json:"author"`
 		UnixTime      int64             `json:"timestamp"`
 		Metadata      map[string]string `json:"metadata,omitempty"`
 		Nonce         []byte            `json:"nonce"`
@@ -188,14 +187,7 @@ func (base *OpBase) UnmarshalJSON(data []byte) error {
 		return err
 	}
 
-	// delegate the decoding of the identity
-	author, err := identity.UnmarshalJSON(aux.Author)
-	if err != nil {
-		return err
-	}
-
 	base.OperationType = aux.OperationType
-	base.Author_ = author
 	base.UnixTime = aux.UnixTime
 	base.Metadata = aux.Metadata
 	base.Nonce = aux.Nonce

migration3/after/bug/operation.go

@@ -10,7 +10,7 @@ import (
 )
 
 type TimelineItem interface {
-	// ID return the identifier of the item
+	// Id return the identifier of the item
 	Id() entity.Id
 }
 
@@ -26,6 +26,7 @@ type CommentHistoryStep struct {
 
 // CommentTimelineItem is a TimelineItem that holds a Comment and its edition history
 type CommentTimelineItem struct {
+	// id should be the same as in Comment
 	id        entity.Id
 	Author    identity.Interface
 	Message   string
@@ -35,9 +36,9 @@ type CommentTimelineItem struct {
 	History   []CommentHistoryStep
 }
 
-func NewCommentTimelineItem(ID entity.Id, comment Comment) CommentTimelineItem {
+func NewCommentTimelineItem(comment Comment) CommentTimelineItem {
 	return CommentTimelineItem{
-		id:        ID,
+		id:        comment.id,
 		Author:    comment.Author,
 		Message:   comment.Message,
 		Files:     comment.Files,

migration3/after/bug/timeline.go

@@ -21,12 +21,12 @@ const editClockPattern = "%s-edit"
 
 // Definition hold the details defining one specialization of an Entity.
 type Definition struct {
-	// the name of the entity (bug, pull-request, ...)
+	// the name of the entity (bug, pull-request, ...), for human consumption
 	Typename string
-	// the Namespace in git (bugs, prs, ...)
+	// the Namespace in git references (bugs, prs, ...)
 	Namespace string
 	// a function decoding a JSON message into an Operation
-	OperationUnmarshaler func(author identity.Interface, raw json.RawMessage) (Operation, error)
+	OperationUnmarshaler func(author identity.Interface, raw json.RawMessage, resolver identity.Resolver) (Operation, error)
 	// the expected format version number, that can be used for data migration/upgrade
 	FormatVersion uint
 }
@@ -121,7 +121,7 @@ func read(def Definition, repo repository.ClockedRepo, resolver identity.Resolve
 
 	// Next step is to:
 	// 1) read the operationPacks
-	// 2) make sure that the clocks causality respect the DAG topology.
+	// 2) make sure that clocks causality respect the DAG topology.
 
 	oppMap := make(map[repository.Hash]*operationPack)
 	var opsCount int
@@ -205,7 +205,7 @@ func read(def Definition, repo repository.ClockedRepo, resolver identity.Resolve
 		if oppSlice[i].EditTime != oppSlice[j].EditTime {
 			return oppSlice[i].EditTime < oppSlice[j].EditTime
 		}
-		// We have equal EditTime, which means we have concurrent edition over different machines and we
+		// We have equal EditTime, which means we have concurrent edition over different machines, and we
 		// can't tell which one came first. So, what now? We still need a total ordering and the most stable possible.
 		// As a secondary ordering, we can order based on a hash of the serialized Operations in the
 		// operationPack. It doesn't carry much meaning but it's unbiased and hard to abuse.

migration3/after/entity/dag/entity.go

@@ -89,7 +89,7 @@ func MergeAll(def Definition, repo repository.ClockedRepo, resolver identity.Res
 	return out
 }
 
-// merge perform a merge to make sure a local Entity is up to date.
+// 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, resolver identity.Resolver, remoteRef string, author identity.Interface) entity.MergeResult {
 	id := entity.RefToId(remoteRef)