diff --git a/api/graphql/resolvers/mutation.go b/api/graphql/resolvers/mutation.go index aa99e93d9..799111183 100644 --- a/api/graphql/resolvers/mutation.go +++ b/api/graphql/resolvers/mutation.go @@ -187,10 +187,22 @@ func (r mutationResolver) EditComment(ctx context.Context, input models.EditComm return nil, err } + // NOTE input.Target is here the comment id (a timeline item id) so must + // split the id, to get the operation id prefix. + _, opIdPrefix := entity.SeparateIds(string(input.Target)) + var opId entity.Id + // Find the full operation id via the prefix + for _, operation := range b.Snapshot().Operations { + if operation.Id().HasPrefix(opIdPrefix) { + opId = operation.Id() + break + } + } + op, err := b.EditCommentRaw( author, time.Now().Unix(), - entity.Id(input.Target), + opId, text.Cleanup(input.Message), nil, ) diff --git a/bridge/gitlab/import.go b/bridge/gitlab/import.go index 79a92dac9..68e3c5201 100644 --- a/bridge/gitlab/import.go +++ b/bridge/gitlab/import.go @@ -217,17 +217,29 @@ func (gi *gitlabImporter) ensureNote(repo *cache.RepoCache, b *cache.BugCache, n case NOTE_DESCRIPTION_CHANGED: issue := gi.iterator.IssueValue() + snap := b.Snapshot() + firstComment := snap.Comments[0] - firstComment := b.Snapshot().Comments[0] // since gitlab doesn't provide the issue history // we should check for "changed the description" notes and compare issue texts // TODO: Check only one time and ignore next 'description change' within one issue if errResolve == cache.ErrNoMatchingOp && issue.Description != firstComment.Message { // comment edition + + _, opIdPrefix := entity.SeparateIds(string(firstComment.Id())) + var opId entity.Id + // Find the full operation id via the extracted prefix + for _, operation := range snap.Operations { + if operation.Id().HasPrefix(opIdPrefix) { + opId = operation.Id() + break + } + } + op, err := b.EditCommentRaw( author, note.UpdatedAt.Unix(), - firstComment.Id(), + opId, text.Cleanup(issue.Description), map[string]string{ metaKeyGitlabId: gitlabID, @@ -265,19 +277,32 @@ func (gi *gitlabImporter) ensureNote(repo *cache.RepoCache, b *cache.BugCache, n // if comment was already exported - // search for last comment update - comment, err := b.Snapshot().SearchComment(id) + //NOTE Is the result of ResolveOperation realy a comment id or an + // operation id? Asking because id is retrieved via + // b.ResolveOperationWithMetadata(metaKeyGitlabId, gitlabID) + // which should return an operation id? + comment, err := b.Snapshot().SearchComment(entity.CombinedId(id)) if err != nil { return err } + _, opIdPrefix := entity.SeparateIds(string(comment.Id())) + var opId entity.Id + // Find the full operation id via the extracted prefix + for _, operation := range b.Snapshot().Operations { + if operation.Id().HasPrefix(opIdPrefix) { + opId = operation.Id() + break + } + } + // compare local bug comment with the new note body if comment.Message != cleanText { // comment edition op, err := b.EditCommentRaw( author, note.UpdatedAt.Unix(), - comment.Id(), + opId, cleanText, nil, ) diff --git a/bug/comment.go b/bug/comment.go index c1cfc7e53..00c6b9c55 100644 --- a/bug/comment.go +++ b/bug/comment.go @@ -13,7 +13,7 @@ import ( type Comment struct { // id should be the result of entity.CombineIds with the Bug id and the id // of the Operation that created the comment - id entity.Id + id entity.CombinedId Author identity.Interface Message string Files []repository.Hash @@ -24,7 +24,7 @@ type Comment struct { } // Id return the Comment identifier -func (c Comment) Id() entity.Id { +func (c Comment) Id() entity.CombinedId { if c.id == "" { // simply panic as it would be a coding error (no id provided at construction) panic("no id") diff --git a/bug/op_label_change.go b/bug/op_label_change.go index 8b0e5ec89..f423f97ef 100644 --- a/bug/op_label_change.go +++ b/bug/op_label_change.go @@ -58,7 +58,7 @@ AddLoop: }) item := &LabelChangeTimelineItem{ - id: op.Id(), + id: entity.CombineIds(snapshot.Id(), op.Id()), Author: op.Author_, UnixTime: timestamp.Timestamp(op.UnixTime), Added: op.Added, @@ -133,14 +133,14 @@ func NewLabelChangeOperation(author identity.Interface, unixTime int64, added, r } type LabelChangeTimelineItem struct { - id entity.Id + id entity.CombinedId Author identity.Interface UnixTime timestamp.Timestamp Added []Label Removed []Label } -func (l LabelChangeTimelineItem) Id() entity.Id { +func (l LabelChangeTimelineItem) Id() entity.CombinedId { return l.id } @@ -208,7 +208,7 @@ func ChangeLabels(b Interface, author identity.Interface, unixTime int64, add, r } // ForceChangeLabels is a convenience function to apply the operation -// The difference with ChangeLabels is that no checks of deduplications are done. You are entirely +// The difference with ChangeLabels is that no checks for deduplication are done. You are entirely // responsible of what you are doing. In the general case, you want to use ChangeLabels instead. // The intended use of this function is to allow importers to create legal but unexpected label changes, // like removing a label with no information of when it was added before. diff --git a/bug/op_set_status.go b/bug/op_set_status.go index e22ded54c..d0d8a552f 100644 --- a/bug/op_set_status.go +++ b/bug/op_set_status.go @@ -27,7 +27,7 @@ func (op *SetStatusOperation) Apply(snapshot *Snapshot) { snapshot.addActor(op.Author_) item := &SetStatusTimelineItem{ - id: op.Id(), + id: entity.CombineIds(snapshot.Id(), op.Id()), Author: op.Author_, UnixTime: timestamp.Timestamp(op.UnixTime), Status: op.Status, @@ -86,13 +86,13 @@ func NewSetStatusOp(author identity.Interface, unixTime int64, status Status) *S } type SetStatusTimelineItem struct { - id entity.Id + id entity.CombinedId Author identity.Interface UnixTime timestamp.Timestamp Status Status } -func (s SetStatusTimelineItem) Id() entity.Id { +func (s SetStatusTimelineItem) Id() entity.CombinedId { return s.id } diff --git a/bug/op_set_title.go b/bug/op_set_title.go index badd192c9..7209c5d32 100644 --- a/bug/op_set_title.go +++ b/bug/op_set_title.go @@ -29,7 +29,7 @@ func (op *SetTitleOperation) Apply(snapshot *Snapshot) { snapshot.addActor(op.Author_) item := &SetTitleTimelineItem{ - id: op.Id(), + id: entity.CombineIds(snapshot.Id(), op.Id()), Author: op.Author_, UnixTime: timestamp.Timestamp(op.UnixTime), Title: op.Title, @@ -100,14 +100,14 @@ func NewSetTitleOp(author identity.Interface, unixTime int64, title string, was } type SetTitleTimelineItem struct { - id entity.Id + id entity.CombinedId Author identity.Interface UnixTime timestamp.Timestamp Title string Was string } -func (s SetTitleTimelineItem) Id() entity.Id { +func (s SetTitleTimelineItem) Id() entity.CombinedId { return s.id } diff --git a/bug/snapshot.go b/bug/snapshot.go index ce84cce12..3b1cf5631 100644 --- a/bug/snapshot.go +++ b/bug/snapshot.go @@ -50,7 +50,7 @@ func (snap *Snapshot) GetCreateMetadata(key string) (string, bool) { } // SearchTimelineItem will search in the timeline for an item matching the given hash -func (snap *Snapshot) SearchTimelineItem(id entity.Id) (TimelineItem, error) { +func (snap *Snapshot) SearchTimelineItem(id entity.CombinedId) (TimelineItem, error) { for i := range snap.Timeline { if snap.Timeline[i].Id() == id { return snap.Timeline[i], nil @@ -61,7 +61,7 @@ func (snap *Snapshot) SearchTimelineItem(id entity.Id) (TimelineItem, error) { } // SearchComment will search for a comment matching the given hash -func (snap *Snapshot) SearchComment(id entity.Id) (*Comment, error) { +func (snap *Snapshot) SearchComment(id entity.CombinedId) (*Comment, error) { for _, c := range snap.Comments { if c.id == id { return &c, nil diff --git a/bug/timeline.go b/bug/timeline.go index a5ca4da5a..765ebc546 100644 --- a/bug/timeline.go +++ b/bug/timeline.go @@ -11,7 +11,7 @@ import ( type TimelineItem interface { // ID return the identifier of the item - Id() entity.Id + Id() entity.CombinedId } // CommentHistoryStep hold one version of a message in the history @@ -27,7 +27,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 + id entity.CombinedId Author identity.Interface Message string Files []repository.Hash @@ -53,7 +53,7 @@ func NewCommentTimelineItem(comment Comment) CommentTimelineItem { } } -func (c *CommentTimelineItem) Id() entity.Id { +func (c *CommentTimelineItem) Id() entity.CombinedId { return c.id } diff --git a/cache/repo_cache_bug.go b/cache/repo_cache_bug.go index c019da68a..cb7c53244 100644 --- a/cache/repo_cache_bug.go +++ b/cache/repo_cache_bug.go @@ -263,7 +263,7 @@ func (c *RepoCache) resolveBugMatcher(f func(*BugExcerpt) bool) (entity.Id, erro // ResolveComment search for a Bug/Comment combination matching the merged // bug/comment Id prefix. Returns the Bug containing the Comment and the Comment's // Id. -func (c *RepoCache) ResolveComment(prefix string) (*BugCache, entity.Id, error) { +func (c *RepoCache) ResolveComment(prefix string) (*BugCache, entity.CombinedId, error) { bugPrefix, _ := entity.SeparateIds(prefix) bugCandidate := make([]entity.Id, 0, 5) @@ -277,7 +277,7 @@ func (c *RepoCache) ResolveComment(prefix string) (*BugCache, entity.Id, error) c.muBug.RUnlock() matchingBugIds := make([]entity.Id, 0, 5) - matchingCommentId := entity.UnsetId + matchingCommentId := entity.UnsetCombinedId var matchingBug *BugCache // search for matching comments @@ -286,7 +286,7 @@ func (c *RepoCache) ResolveComment(prefix string) (*BugCache, entity.Id, error) for _, bugId := range bugCandidate { b, err := c.ResolveBug(bugId) if err != nil { - return nil, entity.UnsetId, err + return nil, entity.UnsetCombinedId, err } for _, comment := range b.Snapshot().Comments { @@ -299,9 +299,9 @@ func (c *RepoCache) ResolveComment(prefix string) (*BugCache, entity.Id, error) } if len(matchingBugIds) > 1 { - return nil, entity.UnsetId, entity.NewErrMultipleMatch("bug/comment", matchingBugIds) + return nil, entity.UnsetCombinedId, entity.NewErrMultipleMatch("bug/comment", matchingBugIds) } else if len(matchingBugIds) == 0 { - return nil, entity.UnsetId, errors.New("comment doesn't exist") + return nil, entity.UnsetCombinedId, errors.New("comment doesn't exist") } return matchingBug, matchingCommentId, nil diff --git a/commands/comment_edit.go b/commands/comment_edit.go index 759ea1948..377bf190e 100644 --- a/commands/comment_edit.go +++ b/commands/comment_edit.go @@ -3,6 +3,7 @@ package commands import ( "github.com/spf13/cobra" + "github.com/MichaelMure/git-bug/entity" "github.com/MichaelMure/git-bug/input" ) @@ -67,7 +68,17 @@ func runCommentEdit(env *Env, opts commentEditOptions, args []string) error { } } - _, err = b.EditComment(commentId, opts.message) + _, opIdPrefix := entity.SeparateIds(string(commentId)) + var opId entity.Id + // Find the full operation id via the extracted prefix + for _, operation := range b.Snapshot().Operations { + if operation.Id().HasPrefix(opIdPrefix) { + opId = operation.Id() + break + } + } + + _, err = b.EditComment(opId, opts.message) if err != nil { return err } diff --git a/entity/id.go b/entity/id.go index c8dbdb94d..4ff16626c 100644 --- a/entity/id.go +++ b/entity/id.go @@ -79,21 +79,3 @@ func (i Id) Validate() error { } return nil } - -/* - * Sorting - */ - -type Alphabetical []Id - -func (a Alphabetical) Len() int { - return len(a) -} - -func (a Alphabetical) Less(i, j int) bool { - return a[i] < a[j] -} - -func (a Alphabetical) Swap(i, j int) { - a[i], a[j] = a[j], a[i] -} diff --git a/entity/id_interleaved.go b/entity/id_interleaved.go index 5423afeed..1e38e9632 100644 --- a/entity/id_interleaved.go +++ b/entity/id_interleaved.go @@ -1,9 +1,73 @@ package entity import ( + "fmt" + "io" "strings" + + "github.com/pkg/errors" ) +const UnsetCombinedId = CombinedId("unset") + +// CombinedId is an Id holding information from both a primary Id and a secondary Id. +// While it looks like a regular Id, do not just cast from one to another. +// Instead, use CombineIds and SeparateIds to create it and split it. +type CombinedId string + +// String return the identifier as a string +func (ci CombinedId) String() string { + return string(ci) +} + +// Human return the identifier, shortened for human consumption +func (ci CombinedId) Human() string { + format := fmt.Sprintf("%%.%ds", humanIdLength) + return fmt.Sprintf(format, ci) +} + +func (ci CombinedId) HasPrefix(prefix string) bool { + return strings.HasPrefix(string(ci), prefix) +} + +// UnmarshalGQL implement the Unmarshaler interface for gqlgen +func (ci *CombinedId) UnmarshalGQL(v interface{}) error { + _, ok := v.(string) + if !ok { + return fmt.Errorf("IDs must be strings") + } + + *ci = v.(CombinedId) + + if err := ci.Validate(); err != nil { + return errors.Wrap(err, "invalid ID") + } + + return nil +} + +// MarshalGQL implement the Marshaler interface for gqlgen +func (ci CombinedId) MarshalGQL(w io.Writer) { + _, _ = w.Write([]byte(`"` + ci.String() + `"`)) +} + +// IsValid tell if the Id is valid +func (ci CombinedId) Validate() error { + // Special case to detect outdated repo + if len(ci) == 40 { + return fmt.Errorf("outdated repository format, please use https://github.com/MichaelMure/git-bug-migration to upgrade") + } + if len(ci) != idLength { + return fmt.Errorf("invalid length") + } + for _, r := range ci { + if (r < 'a' || r > 'z') && (r < '0' || r > '9') { + return fmt.Errorf("invalid character") + } + } + return nil +} + // CombineIds compute a merged Id holding information from both the primary Id // and the secondary Id. // @@ -32,7 +96,7 @@ import ( // 7: 4P, 3S // 10: 6P, 4S // 16: 11P, 5S -func CombineIds(primary Id, secondary Id) Id { +func CombineIds(primary Id, secondary Id) CombinedId { var id strings.Builder for i := 0; i < idLength; i++ { @@ -46,7 +110,7 @@ func CombineIds(primary Id, secondary Id) Id { } } - return Id(id.String()) + return CombinedId(id.String()) } // SeparateIds extract primary and secondary prefix from an arbitrary length prefix diff --git a/entity/id_interleaved_test.go b/entity/id_interleaved_test.go index ef9218c97..1dc791457 100644 --- a/entity/id_interleaved_test.go +++ b/entity/id_interleaved_test.go @@ -9,7 +9,7 @@ import ( func TestInterleaved(t *testing.T) { primary := Id("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWX______________") secondary := Id("YZ0123456789+/________________________________________________") - expectedId := Id("aYbZc0def1ghij2klmn3opqr4stuv5wxyz6ABCD7EFGH8IJKL9MNOP+QRST/UVWX") + expectedId := CombinedId("aYbZc0def1ghij2klmn3opqr4stuv5wxyz6ABCD7EFGH8IJKL9MNOP+QRST/UVWX") interleaved := CombineIds(primary, secondary) require.Equal(t, expectedId, interleaved) diff --git a/termui/show_bug.go b/termui/show_bug.go index 0710fa34b..e25017eb9 100644 --- a/termui/show_bug.go +++ b/termui/show_bug.go @@ -645,16 +645,26 @@ func (sb *showBug) edit(g *gocui.Gui, v *gocui.View) error { return nil } - op, err := snap.SearchTimelineItem(entity.Id(sb.selected)) + item, err := snap.SearchTimelineItem(entity.CombinedId(sb.selected)) if err != nil { return err } - switch op := op.(type) { + _, opIdPrefix := entity.SeparateIds(string(item.Id())) + var opId entity.Id + // Find the full operation id via the extracted prefix + for _, operation := range snap.Operations { + if operation.Id().HasPrefix(opIdPrefix) { + opId = operation.Id() + break + } + } + + switch op := item.(type) { case *bug.AddCommentTimelineItem: - return editCommentWithEditor(sb.bug, op.Id(), op.Message) + return editCommentWithEditor(sb.bug, opId, op.Message) case *bug.CreateTimelineItem: - return editCommentWithEditor(sb.bug, op.Id(), op.Message) + return editCommentWithEditor(sb.bug, opId, op.Message) case *bug.LabelChangeTimelineItem: return sb.editLabels(g, snap) }