diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index b23e5b1b1c..6bc339bc61 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1477,38 +1477,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()] - - // Check whether it was previously detected that the file has changed since the last review - if fileViewedState == pull_model.HasChanged { - diffFile.HasChangedSinceLastReview = true - continue - } - filename := diffFile.GetDiffFileName() + fileViewedState := review.UpdatedFiles[filename] - // 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 { + if fileViewedState == pull_model.HasChanged { // Check whether it was previously detected that the file has changed since the last review + diffFile.HasChangedSinceLastReview = true + 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 } } + // 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) diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 62b17c223c..cfd99544cc 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -11,6 +11,7 @@ import ( "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" @@ -1143,3 +1144,96 @@ func TestHighlightCodeLines(t *testing.T) { }, 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 1772749114 +0000 +data 7 +change +from 1978192d98bb1b65e11c2cf37da854fbf94bffd6 +M 100644 :1 test2.txt +M 100644 :1 test3.txt + +commit refs/heads/branch1 +committer test 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()) +}