From 9b70caf7988b8303fd821437877c1e8eb79a5745 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 27 Feb 2024 22:12:20 +0100 Subject: [PATCH] [BUG] Correct changed files for codeowners - Backport of #2507 - The CODEOWNER feature relies on the changed files to determine which reviewers should be added according to the `CODEOWNER` file. - The current approach was to 'diff' between the base and head branch, which seems logical but fail in practice when the pull request is out of date with the base branch. Therefore it should instead diff between the head branch and the merge base of the head and base branch, so only the actual affected files by the pull requests are used, the same approach is used by the diff of an unmerged pull request. - Add integration testing (for the feature as well). - Resolves #2458 (cherry picked from commit fb2795b5bb3a119f022b59a7b46af47eff94270f) --- models/issues/pull.go | 9 +- tests/integration/codeowner_test.go | 152 ++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 tests/integration/codeowner_test.go diff --git a/models/issues/pull.go b/models/issues/pull.go index 3ca0847652..f60b1c5e7a 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -925,7 +925,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullReque } rules, _ := GetCodeOwnersFromContent(ctx, data) - changedFiles, err := repo.GetFilesChangedBetween(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + + prInfo, err := repo.GetCompareInfo(repo.Path, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) + if err != nil { + return err + } + // Use the merge base as the base instead of the main branch to avoid problems + // if the pull request is out of date with the base branch. + changedFiles, err := repo.GetFilesChangedBetween(prInfo.MergeBase, pr.HeadCommitID) if err != nil { return err } diff --git a/tests/integration/codeowner_test.go b/tests/integration/codeowner_test.go new file mode 100644 index 0000000000..90fe1275cc --- /dev/null +++ b/tests/integration/codeowner_test.go @@ -0,0 +1,152 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "context" + "fmt" + "net/url" + "os" + "path" + "strings" + "testing" + "time" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestCodeOwner(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // Create a new repository + repo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ + Name: "code-owner", + Description: "Temporary Repo", + AutoInit: true, + Gitignores: "", + License: "WTFPL", + Readme: "Default", + DefaultBranch: "main", + }) + assert.NoError(t, err) + assert.NotEmpty(t, repo) + + err = repo_model.UpdateRepositoryUnits(repo, []repo_model.RepoUnit{{RepoID: repo.ID, Type: unit.TypePullRequests}}, nil) + assert.NoError(t, err) + + resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "CODEOWNERS", + ContentReader: strings.NewReader("README.md @user5\ntest-file @user4"), + }, + }, + Message: "add files", + OldBranch: "main", + NewBranch: "main", + Author: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + assert.NoError(t, err) + assert.NotEmpty(t, resp) + + dstPath := t.TempDir() + r := fmt.Sprintf("%suser2/%s.git", u.String(), repo.Name) + u, _ = url.Parse(r) + u.User = url.UserPassword("user2", userPassword) + assert.NoError(t, git.CloneWithArgs(context.Background(), nil, u.String(), dstPath, git.CloneRepoOptions{})) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + err := os.WriteFile(path.Join(dstPath, "README.md"), []byte("## test content"), 0o666) + assert.NoError(t, err) + + err = git.AddChanges(dstPath, true) + assert.NoError(t, err) + + err = git.CommitChanges(dstPath, git.CommitChangesOptions{ + Committer: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Author: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Message: "Add README.", + }) + assert.NoError(t, err) + + err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/main", "-o", "topic=codeowner-normal").Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "user2/codeowner-normal"}) + unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + }) + + t.Run("Out of date", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Push the changes made from the previous subtest. + assert.NoError(t, git.NewCommand(git.DefaultContext, "push", "origin").Run(&git.RunOpts{Dir: dstPath})) + + // Reset the tree to the previous commit. + assert.NoError(t, git.NewCommand(git.DefaultContext, "reset", "--hard", "HEAD~1").Run(&git.RunOpts{Dir: dstPath})) + + err := os.WriteFile(path.Join(dstPath, "test-file"), []byte("## test content"), 0o666) + assert.NoError(t, err) + + err = git.AddChanges(dstPath, true) + assert.NoError(t, err) + + err = git.CommitChanges(dstPath, git.CommitChangesOptions{ + Committer: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Author: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Message: "Add test-file.", + }) + assert.NoError(t, err) + + err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/main", "-o", "topic=codeowner-out-of-date").Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "user2/codeowner-out-of-date"}) + unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 4}) + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + }) + }) +}