Avoid re-issuing redundant cross-references. (#8734)

* Avoid re-issuing redundant cross-references.

* Remove unused func; fix lint

* Simplify code as suggested by @laftriks

* Update test

diff --git a/models/issue.go b/models/issue.go
index 0113f0a4046bf63c9cce8fd7e04dbd74342b0dc3..e28e7c01c3025fa4716d8e10e6686d654338d04c 100644 (file)
--- a/models/issue.go
+++ b/models/issue.go
@@ -722,11 +722,7 @@ func (issue *Issue) ChangeTitle(doer *User, oldTitle string) (err error) {
                return fmt.Errorf("createComment: %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
        }
 
@@ -790,10 +786,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
        }
 
@@ -944,7 +938,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.
@@ -1578,13 +1572,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 7ac8c4df4383a8507441d880ceb3ba6920781e89..73fd9c8c83eebe83e0eb1bbb86cc96effc5c8ecf 100644 (file)
--- a/models/issue_comment.go
+++ b/models/issue_comment.go
@@ -545,7 +545,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
                }
        }
 
-       if err = comment.addCrossReferences(e, opts.Doer); err != nil {
+       if err = comment.addCrossReferences(e, opts.Doer, false); err != nil {
                return nil, err
        }
 
@@ -882,10 +882,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 f41c154753068ddfeaa28d83034057ad02a7ce44..9f30aba259178d7eed8f8255788986e81102c635 100644 (file)
--- a/models/issue_xref.go
+++ b/models/issue_xref.go
@@ -25,20 +25,30 @@ type crossReferencesContext struct {
        Doer        *User
        OrigIssue   *Issue
        OrigComment *Comment
+       RemoveOld   bool
 }
 
-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).
+       return active, e.Where("`ref_action` IN (?, ?, ?)", references.XRefActionNone, references.XRefActionCloses, references.XRefActionReopens).
                And("`ref_issue_id` = ?", issueID).
-               And("`ref_comment_id` = ?", commentID)
-       if err := sess.Find(&active); err != nil || len(active) == 0 {
+               And("`ref_comment_id` = ?", commentID).
+               Find(&active)
+}
+
+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
 }
@@ -51,7 +61,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
@@ -62,6 +72,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)
 }
@@ -71,6 +82,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 {
                var refCommentID int64
                if ctx.OrigComment != nil {
@@ -193,10 +233,6 @@ func (issue *Issue) verifyReferencedIssue(e Engine, ctx *crossReferencesContext,
        return refIssue, refAction, nil
 }
 
-func (issue *Issue) neuterCrossReferences(e Engine) error {
-       return neuterCrossReferences(e, issue.ID, 0)
-}
-
 // _________                                       __
 // \_   ___ \  ____   _____   _____   ____   _____/  |_
 // /    \  \/ /  _ \ /     \ /     \_/ __ \ /    \   __\
@@ -205,7 +241,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
        }
@@ -217,6 +253,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)
 }

diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go
index 4fc6011d788a9bba1291bc05c43b2b568f7a234b..c13577e905aeb117994130fa2d950f5104c797b6 100644 (file)
--- 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
 }