Improvements to content history (#17746)

* Improvements to content history

* initialize content history when making an edit to an old item created before the introduction of content history
* show edit history for code comments on pull request files tab

* Fix a flaw in keepLimitedContentHistory
Fix a flaw in keepLimitedContentHistory, the first and the last should never be deleted

* Remove obsolete eager initialization of content history

3 files changed, 38 insertions, 14 deletions

diff --git a/models/issue.go b/models/issue.go
index 6de7c3204b..7aab8b9399 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -824,14 +824,25 @@ func (issue *Issue) UpdateAttachments(uuids []string) (err error) {
 
 // ChangeContent changes issue content, as the given user.
 func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
-	issue.Content = content
-
 	ctx, committer, err := db.TxContext()
 	if err != nil {
 		return err
 	}
 	defer committer.Close()
 
+	hasContentHistory, err := issues.HasIssueContentHistory(ctx, issue.ID, 0)
+	if err != nil {
+		return fmt.Errorf("HasIssueContentHistory: %v", err)
+	}
+	if !hasContentHistory {
+		if err = issues.SaveIssueContentHistory(db.GetEngine(ctx), issue.PosterID, issue.ID, 0,
+			issue.CreatedUnix, issue.Content, true); err != nil {
+			return fmt.Errorf("SaveIssueContentHistory: %v", err)
+		}
+	}
+
+	issue.Content = content
+
 	if err = updateIssueCols(db.GetEngine(ctx), issue, "content"); err != nil {
 		return fmt.Errorf("UpdateIssueCols: %v", err)
 	}
@@ -1012,11 +1023,6 @@ func newIssue(ctx context.Context, doer *User, opts NewIssueOptions) (err error)
 		return err
 	}
 
-	if err = issues.SaveIssueContentHistory(e, doer.ID, opts.Issue.ID, 0,
-		timeutil.TimeStampNow(), opts.Issue.Content, true); err != nil {
-		return err
-	}
-
 	return opts.Issue.addCrossReferences(ctx, doer, false)
 }
 
diff --git a/models/issues/content_history.go b/models/issues/content_history.go
index 04c30fde92..b423fb8fcf 100644
--- a/models/issues/content_history.go
+++ b/models/issues/content_history.go
@@ -75,7 +75,7 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
 		log.Error("can not query content history for deletion, err=%v", err)
 		return
 	}
-	if len(res) <= 1 {
+	if len(res) <= 2 {
 		return
 	}
 
@@ -83,8 +83,8 @@ func keepLimitedContentHistory(e db.Engine, issueID, commentID int64, limit int)
 	for outDatedCount > 0 {
 		var indexToDelete int
 		minEditedInterval := -1
-		// find a history revision with minimal edited interval to delete
-		for i := 1; i < len(res); i++ {
+		// find a history revision with minimal edited interval to delete, the first and the last should never be deleted
+		for i := 1; i < len(res)-1; i++ {
 			editedInterval := int(res[i].EditedUnix - res[i-1].EditedUnix)
 			if minEditedInterval == -1 || editedInterval < minEditedInterval {
 				minEditedInterval = editedInterval
@@ -167,7 +167,20 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID int64, commentI
 	return res, nil
 }
 
-//SoftDeleteIssueContentHistory soft delete
+// HasIssueContentHistory check if a ContentHistory entry exists
+func HasIssueContentHistory(dbCtx context.Context, issueID int64, commentID int64) (bool, error) {
+	exists, err := db.GetEngine(dbCtx).Cols("id").Exist(&ContentHistory{
+		IssueID:   issueID,
+		CommentID: commentID,
+	})
+	if err != nil {
+		log.Error("can not fetch issue content history. err=%v", err)
+		return false, err
+	}
+	return exists, err
+}
+
+// SoftDeleteIssueContentHistory soft delete
 func SoftDeleteIssueContentHistory(dbCtx context.Context, historyID int64) error {
 	if _, err := db.GetEngine(dbCtx).ID(historyID).Cols("is_deleted", "content_text").Update(&ContentHistory{
 		IsDeleted:   true,
diff --git a/models/issues/content_history_test.go b/models/issues/content_history_test.go
index f040a7dc57..cc9a7c5107 100644
--- a/models/issues/content_history_test.go
+++ b/models/issues/content_history_test.go
@@ -53,6 +53,11 @@ func TestContentHistory(t *testing.T) {
 	list2, _ := FetchIssueContentHistoryList(dbCtx, 10, 100)
 	assert.Len(t, list2, 5)
 
+	hasHistory1, _ := HasIssueContentHistory(dbCtx, 10, 0)
+	assert.True(t, hasHistory1)
+	hasHistory2, _ := HasIssueContentHistory(dbCtx, 10, 1)
+	assert.False(t, hasHistory2)
+
 	h6, h6Prev, _ := GetIssueContentHistoryAndPrev(dbCtx, 6)
 	assert.EqualValues(t, 6, h6.ID)
 	assert.EqualValues(t, 5, h6Prev.ID)
@@ -63,13 +68,13 @@ func TestContentHistory(t *testing.T) {
 	assert.EqualValues(t, 6, h6.ID)
 	assert.EqualValues(t, 4, h6Prev.ID)
 
-	// only keep 3 history revisions for comment_id=100
+	// only keep 3 history revisions for comment_id=100, the first and the last should never be deleted
 	keepLimitedContentHistory(dbEngine, 10, 100, 3)
 	list1, _ = FetchIssueContentHistoryList(dbCtx, 10, 0)
 	assert.Len(t, list1, 3)
 	list2, _ = FetchIssueContentHistoryList(dbCtx, 10, 100)
 	assert.Len(t, list2, 3)
-	assert.EqualValues(t, 7, list2[0].HistoryID)
-	assert.EqualValues(t, 6, list2[1].HistoryID)
+	assert.EqualValues(t, 8, list2[0].HistoryID)
+	assert.EqualValues(t, 7, list2[1].HistoryID)
 	assert.EqualValues(t, 4, list2[2].HistoryID)
 }