From 5f8bc831da1acda76573402af3008e4c20b1f38c Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 29 Oct 2019 00:05:53 -0300 Subject: [PATCH 1/4] Avoid re-issuing redundant cross-references. --- models/issue.go | 19 +++++----------- models/issue_comment.go | 7 ++---- models/issue_xref.go | 50 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/models/issue.go b/models/issue.go index b4bd190aa4cf..0f734fce4c42 100644 --- a/models/issue.go +++ b/models/issue.go @@ -699,11 +699,7 @@ func (issue *Issue) ChangeTitle(doer *User, oldTitle string) (err error) { return fmt.Errorf("createChangeTitleComment: %v", err) } - if err = issue.neuterCrossReferences(sess); err != nil { - return err - } - - if err = issue.addCrossReferences(sess, doer); err != nil { + if err = issue.addCrossReferences(sess, doer, true); err != nil { return err } @@ -762,10 +758,8 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) { if err = updateIssueCols(sess, issue, "content"); err != nil { return fmt.Errorf("UpdateIssueCols: %v", err) } - if err = issue.neuterCrossReferences(sess); err != nil { - return err - } - if err = issue.addCrossReferences(sess, doer); err != nil { + + if err = issue.addCrossReferences(sess, doer, true); err != nil { return err } @@ -956,7 +950,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { if err = opts.Issue.loadAttributes(e); err != nil { return err } - return opts.Issue.addCrossReferences(e, doer) + return opts.Issue.addCrossReferences(e, doer, false) } // NewIssue creates new issue with labels for repository. @@ -1568,13 +1562,10 @@ func UpdateIssue(issue *Issue) error { if err := updateIssue(sess, issue); err != nil { return err } - if err := issue.neuterCrossReferences(sess); err != nil { - return err - } if err := issue.loadPoster(sess); err != nil { return err } - if err := issue.addCrossReferences(sess, issue.Poster); err != nil { + if err := issue.addCrossReferences(sess, issue.Poster, true); err != nil { return err } return sess.Commit() diff --git a/models/issue_comment.go b/models/issue_comment.go index d7128bdbac3d..5984d3337e42 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -535,7 +535,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err return nil, err } - if err = comment.addCrossReferences(e, opts.Doer); err != nil { + if err = comment.addCrossReferences(e, opts.Doer, false); err != nil { return nil, err } @@ -921,10 +921,7 @@ func UpdateComment(c *Comment, doer *User) error { if err := c.loadIssue(sess); err != nil { return err } - if err := c.neuterCrossReferences(sess); err != nil { - return err - } - if err := c.addCrossReferences(sess, doer); err != nil { + if err := c.addCrossReferences(sess, doer, true); err != nil { return err } if err := sess.Commit(); err != nil { diff --git a/models/issue_xref.go b/models/issue_xref.go index 4b01022bc575..dc30695a5e32 100644 --- a/models/issue_xref.go +++ b/models/issue_xref.go @@ -23,6 +23,7 @@ type crossReferencesContext struct { Doer *User OrigIssue *Issue OrigComment *Comment + RemoveOld bool } func newCrossReference(e *xorm.Session, ctx *crossReferencesContext, xref *crossReference) error { @@ -44,7 +45,7 @@ func newCrossReference(e *xorm.Session, ctx *crossReferencesContext, xref *cross return err } -func neuterCrossReferences(e Engine, issueID int64, commentID int64) error { +func findOldCrossReferences(e Engine, issueID int64, commentID int64) ([]*Comment, error) { active := make([]*Comment, 0, 10) sess := e.Where("`ref_action` IN (?, ?, ?)", references.XRefActionNone, references.XRefActionCloses, references.XRefActionReopens) if issueID != 0 { @@ -53,13 +54,23 @@ func neuterCrossReferences(e Engine, issueID int64, commentID int64) error { if commentID != 0 { sess = sess.And("`ref_comment_id` = ?", commentID) } - if err := sess.Find(&active); err != nil || len(active) == 0 { + err := sess.Find(&active) + return active, err +} + +func neuterCrossReferences(e Engine, issueID int64, commentID int64) error { + active, err := findOldCrossReferences(e, issueID, commentID) + if err != nil { return err } ids := make([]int64, len(active)) for i, c := range active { ids[i] = c.ID } + return neuterCrossReferencesIds(e, ids) +} + +func neuterCrossReferencesIds(e Engine, ids []int64) error { _, err := e.In("id", ids).Cols("`ref_action`").Update(&Comment{RefAction: references.XRefActionNeutered}) return err } @@ -72,7 +83,7 @@ func neuterCrossReferences(e Engine, issueID int64, commentID int64) error { // \/ \/ \/ // -func (issue *Issue) addCrossReferences(e *xorm.Session, doer *User) error { +func (issue *Issue) addCrossReferences(e *xorm.Session, doer *User, removeOld bool) error { var commentType CommentType if issue.IsPull { commentType = CommentTypePullRef @@ -83,6 +94,7 @@ func (issue *Issue) addCrossReferences(e *xorm.Session, doer *User) error { Type: commentType, Doer: doer, OrigIssue: issue, + RemoveOld: removeOld, } return issue.createCrossReferences(e, ctx, issue.Title, issue.Content) } @@ -92,6 +104,35 @@ func (issue *Issue) createCrossReferences(e *xorm.Session, ctx *crossReferencesC if err != nil { return err } + if ctx.RemoveOld { + var commentId int64 + if ctx.OrigComment != nil { + commentId = ctx.OrigComment.ID + } + active, err := findOldCrossReferences(e, ctx.OrigIssue.ID, commentId) + if err != nil { + return err + } + ids := make([]int64, 0, len(active)) + for _, c := range active { + found := false + for i, x := range xreflist { + if x.Issue.ID == c.IssueID && x.Action == c.RefAction { + found = true + xreflist = append(xreflist[:i], xreflist[i+1:]...) + break + } + } + if !found { + ids = append(ids, c.ID) + } + } + if len(ids) > 0 { + if err = neuterCrossReferencesIds(e, ids); err != nil { + return err + } + } + } for _, xref := range xreflist { if err = newCrossReference(e, ctx, xref); err != nil { return err @@ -191,7 +232,7 @@ func (issue *Issue) neuterCrossReferences(e Engine) error { // \/ \/ \/ \/ \/ // -func (comment *Comment) addCrossReferences(e *xorm.Session, doer *User) error { +func (comment *Comment) addCrossReferences(e *xorm.Session, doer *User, removeOld bool) error { if comment.Type != CommentTypeCode && comment.Type != CommentTypeComment { return nil } @@ -203,6 +244,7 @@ func (comment *Comment) addCrossReferences(e *xorm.Session, doer *User) error { Doer: doer, OrigIssue: comment.Issue, OrigComment: comment, + RemoveOld: removeOld, } return comment.Issue.createCrossReferences(e, ctx, "", comment.Content) } From b90bb3ec3f5b5d31262368254a5c9e778e64ef32 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 29 Oct 2019 01:11:17 -0300 Subject: [PATCH 2/4] Remove unused func; fix lint --- models/issue_xref.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/models/issue_xref.go b/models/issue_xref.go index dc30695a5e32..38c54bfaf354 100644 --- a/models/issue_xref.go +++ b/models/issue_xref.go @@ -105,11 +105,11 @@ func (issue *Issue) createCrossReferences(e *xorm.Session, ctx *crossReferencesC return err } if ctx.RemoveOld { - var commentId int64 + var commentID int64 if ctx.OrigComment != nil { - commentId = ctx.OrigComment.ID + commentID = ctx.OrigComment.ID } - active, err := findOldCrossReferences(e, ctx.OrigIssue.ID, commentId) + active, err := findOldCrossReferences(e, ctx.OrigIssue.ID, commentID) if err != nil { return err } @@ -220,10 +220,6 @@ func (issue *Issue) findReferencedIssue(e Engine, ctx *crossReferencesContext, r return refIssue, nil } -func (issue *Issue) neuterCrossReferences(e Engine) error { - return neuterCrossReferences(e, issue.ID, 0) -} - // _________ __ // \_ ___ \ ____ _____ _____ ____ _____/ |_ // / \ \/ / _ \ / \ / \_/ __ \ / \ __\ From 0f1b0e781f09492a03d8d4c00d81289377696c56 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 29 Oct 2019 11:45:29 -0300 Subject: [PATCH 3/4] Simplify code as suggested by @laftriks --- models/issue_xref.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/models/issue_xref.go b/models/issue_xref.go index 38c54bfaf354..dad7d58f042b 100644 --- a/models/issue_xref.go +++ b/models/issue_xref.go @@ -54,8 +54,7 @@ func findOldCrossReferences(e Engine, issueID int64, commentID int64) ([]*Commen if commentID != 0 { sess = sess.And("`ref_comment_id` = ?", commentID) } - err := sess.Find(&active) - return active, err + return active, sess.Find(&active) } func neuterCrossReferences(e Engine, issueID int64, commentID int64) error { From 85219e0ec7c10d91a999da49552cdcbbb8703a3e Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Mon, 18 Nov 2019 12:30:22 -0300 Subject: [PATCH 4/4] Update test --- models/issue_xref_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index 4fc6011d788a..c13577e905ae 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -133,7 +133,7 @@ func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispu assert.NoError(t, err) i, err = getIssueByID(sess, i.ID) assert.NoError(t, err) - assert.NoError(t, i.addCrossReferences(sess, d)) + assert.NoError(t, i.addCrossReferences(sess, d, false)) assert.NoError(t, sess.Commit()) return i } @@ -158,7 +158,7 @@ func testCreateComment(t *testing.T, repo, doer, issue int64, content string) *C assert.NoError(t, sess.Begin()) _, err := sess.Insert(c) assert.NoError(t, err) - assert.NoError(t, c.addCrossReferences(sess, d)) + assert.NoError(t, c.addCrossReferences(sess, d, false)) assert.NoError(t, sess.Commit()) return c }