Fix incorrect viewed files counter if reverted change was viewed (#36819)

services/gitdiff/gitdiff.go

@@ -1342,38 +1342,40 @@ func SyncUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.
 	if errIgnored != nil {
 		log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, gitRepo.Path, err)
 	}
+	changedFilesSet := make(map[string]struct{}, len(changedFiles))
+	for _, changedFile := range changedFiles {
+		changedFilesSet[changedFile] = struct{}{}
+	}
 
 	filesChangedSinceLastDiff := make(map[string]pull_model.ViewedState)
-outer:
 	for _, diffFile := range diff.Files {
-		fileViewedState := review.UpdatedFiles[diffFile.GetDiffFileName()]
+		filename := diffFile.GetDiffFileName()
+		fileViewedState := review.UpdatedFiles[filename]
 
-		// Check whether it was previously detected that the file has changed since the last review
-		if fileViewedState == pull_model.HasChanged {
+		if fileViewedState == pull_model.HasChanged { // Check whether it was previously detected that the file has changed since the last review
 			diffFile.HasChangedSinceLastReview = true
-			continue
+			delete(changedFilesSet, filename)
+		} else if _, ok := changedFilesSet[filename]; ok { // Check explicitly whether the file has changed since the last review
+			diffFile.HasChangedSinceLastReview = true
+			filesChangedSinceLastDiff[filename] = pull_model.HasChanged
+			delete(changedFilesSet, filename)
+		} else if fileViewedState == pull_model.Viewed { // Check whether the file has already been viewed
+			diffFile.IsViewed = true
 		}
+	}
 
-		filename := diffFile.GetDiffFileName()
-
-		// Check explicitly whether the file has changed since the last review
-		for _, changedFile := range changedFiles {
-			diffFile.HasChangedSinceLastReview = filename == changedFile
-			if diffFile.HasChangedSinceLastReview {
-				filesChangedSinceLastDiff[filename] = pull_model.HasChanged
-				continue outer // We don't want to check if the file is viewed here as that would fold the file, which is in this case unwanted
-			}
-		}
-		// Check whether the file has already been viewed
-		if fileViewedState == pull_model.Viewed {
-			diffFile.IsViewed = true
+	// All changed files still present at this point aren't part of the diff anymore, this occurs
+	// when a file was modified in a previous commit of the diff and the modification got reverted afterwards.
+	// Marking the files as unviewed to prevent errors where a non-existing file has a view state
+	for changedFile := range changedFilesSet {
+		if _, ok := review.UpdatedFiles[changedFile]; ok {
+			filesChangedSinceLastDiff[changedFile] = pull_model.Unviewed
 		}
 	}
 
 	if len(filesChangedSinceLastDiff) > 0 {
 		// Explicitly store files that have changed in the database, if any is present at all.
 		// This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed.
-		// On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed.
 		updatedReview, err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff)
 		if err != nil {
 			log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err)

services/gitdiff/gitdiff_test.go

@@ -11,6 +11,7 @@
 	"testing"
 
 	issues_model "code.gitea.io/gitea/models/issues"
+	pull_model "code.gitea.io/gitea/models/pull"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
@@ -679,3 +680,96 @@
 		}, ret)
 	})
 }
+
+func TestSyncUserSpecificDiff_UpdatedFiles(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 7})
+	assert.NoError(t, pull.LoadBaseRepo(t.Context()))
+
+	stdin := `blob
+mark :1
+data 7
+change
+
+commit refs/heads/branch1
+mark :2
+committer test <test@example.com> 1772749114 +0000
+data 7
+change
+from 1978192d98bb1b65e11c2cf37da854fbf94bffd6
+M 100644 :1 test2.txt
+M 100644 :1 test3.txt
+
+commit refs/heads/branch1
+committer test <test@example.com> 1772749114 +0000
+data 7
+revert
+from :2
+D test2.txt
+D test10.txt`
+	require.NoError(t, gitcmd.NewCommand("fast-import").WithDir(pull.BaseRepo.RepoPath()).WithStdinBytes([]byte(stdin)).Run(t.Context()))
+
+	gitRepo, err := git.OpenRepository(t.Context(), pull.BaseRepo.RepoPath())
+	assert.NoError(t, err)
+	defer gitRepo.Close()
+
+	firstReviewCommit := "1978192d98bb1b65e11c2cf37da854fbf94bffd6"
+	firstReviewUpdatedFiles := map[string]pull_model.ViewedState{
+		"test1.txt":  pull_model.Viewed,
+		"test2.txt":  pull_model.Viewed,
+		"test10.txt": pull_model.Viewed,
+	}
+	_, err = pull_model.UpdateReviewState(t.Context(), user.ID, pull.ID, firstReviewCommit, firstReviewUpdatedFiles)
+	assert.NoError(t, err)
+	firstReview, err := pull_model.GetNewestReviewState(t.Context(), user.ID, pull.ID)
+	assert.NoError(t, err)
+	assert.NotNil(t, firstReview)
+	assert.Equal(t, firstReviewUpdatedFiles, firstReview.UpdatedFiles)
+	assert.Equal(t, 3, firstReview.GetViewedFileCount())
+
+	secondReviewCommit := "f80737c7dc9de0a9c1e051e83cb6897f950c6bb8"
+	secondReviewUpdatedFiles := map[string]pull_model.ViewedState{
+		"test1.txt":  pull_model.Viewed,
+		"test2.txt":  pull_model.HasChanged,
+		"test3.txt":  pull_model.HasChanged,
+		"test10.txt": pull_model.Viewed,
+	}
+	secondReviewDiffOpts := &DiffOptions{
+		AfterCommitID:     secondReviewCommit,
+		BeforeCommitID:    pull.MergeBase,
+		MaxLines:          setting.Git.MaxGitDiffLines,
+		MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
+		MaxFiles:          setting.Git.MaxGitDiffFiles,
+	}
+	secondReviewDiff, err := GetDiffForAPI(t.Context(), gitRepo, secondReviewDiffOpts)
+	assert.NoError(t, err)
+	secondReview, err := SyncUserSpecificDiff(t.Context(), user.ID, pull, gitRepo, secondReviewDiff, secondReviewDiffOpts)
+	assert.NoError(t, err)
+	assert.NotNil(t, secondReview)
+	assert.Equal(t, secondReviewUpdatedFiles, secondReview.UpdatedFiles)
+	assert.Equal(t, 2, secondReview.GetViewedFileCount())
+
+	thirdReviewCommit := "73424f3a99e140f6399c73a1712654e122d2a74b"
+	thirdReviewUpdatedFiles := map[string]pull_model.ViewedState{
+		"test1.txt":  pull_model.Viewed,
+		"test2.txt":  pull_model.Unviewed,
+		"test3.txt":  pull_model.HasChanged,
+		"test10.txt": pull_model.Unviewed,
+	}
+	thirdReviewDiffOpts := &DiffOptions{
+		AfterCommitID:     thirdReviewCommit,
+		BeforeCommitID:    pull.MergeBase,
+		MaxLines:          setting.Git.MaxGitDiffLines,
+		MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
+		MaxFiles:          setting.Git.MaxGitDiffFiles,
+	}
+	thirdReviewDiff, err := GetDiffForAPI(t.Context(), gitRepo, thirdReviewDiffOpts)
+	assert.NoError(t, err)
+	thirdReview, err := SyncUserSpecificDiff(t.Context(), user.ID, pull, gitRepo, thirdReviewDiff, thirdReviewDiffOpts)
+	assert.NoError(t, err)
+	assert.NotNil(t, thirdReview)
+	assert.Equal(t, thirdReviewUpdatedFiles, thirdReview.UpdatedFiles)
+	assert.Equal(t, 1, thirdReview.GetViewedFileCount())
+}