Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CODEOWNERS test against diff towards HEAD of target branch instead of merge-base #29763

Closed
baronjeppe opened this issue Mar 13, 2024 · 0 comments · Fixed by #29783
Closed

CODEOWNERS test against diff towards HEAD of target branch instead of merge-base #29763

baronjeppe opened this issue Mar 13, 2024 · 0 comments · Fixed by #29783
Labels
type/bug
Milestone
1.21.9

Comments

@baronjeppe
Copy link

Description

When a new PR is opened the CODEOWNERS functionality tests against the diff towards refs/heads/ as seen here:

gitea/models/issues/pull.go

Line 920 in 7fd0a5b

changedFiles, err := repo.GetFilesChangedBetween(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())

This means that if the source branch for the PR is not up to date with the target the diff will contain all the changes added to the target branch since merge base - this seems like the wrong behavior to me.

I think it should use merge base instead of head - which i also assume is the logic used to determine which changes needs to be reviewed in a PR.

I am not familiar with the codebase of gitea, so i am not comfortable make the pr to change this, but if it is agreed that this change should be made, and nobody else has the bandwidth to do it, i might give it a try.

Gitea Version

1.21.8

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

Not relevant

Database

MySQL/MariaDB
@lunny lunny added this to the 1.21.9 milestone Mar 13, 2024
@lunny lunny mentioned this issue Mar 14, 2024
Merged
lunny added a commit that referenced this issue Mar 15, 2024
@lunny @wxiaoguang
Fix codeowner detected diff base branch to mergebase (#29783)
277f90d 
Fix #29763

This PR fixes 2 problems with CodeOwner in the pull request.
- Don't use the pull request base branch but merge-base as a diff base to
detect the code owner.
- CodeOwner detection in fork repositories will be disabled because
almost all the fork repositories will not change CODEOWNERS files but it
should not be used on fork repositories' pull requests.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
lunny added a commit to lunny/gitea that referenced this issue Mar 15, 2024
@lunny @wxiaoguang
Fix codeowner detected diff base branch to mergebase (go-gitea#29783)
d30950d 
Fix go-gitea#29763

This PR fixes 2 problems with CodeOwner in the pull request.
- Don't use the pull request base branch but merge-base as a diff base to
detect the code owner.
- CodeOwner detection in fork repositories will be disabled because
almost all the fork repositories will not change CODEOWNERS files but it
should not be used on fork repositories' pull requests.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@lunny lunny mentioned this issue Mar 15, 2024
Merged
6543 pushed a commit that referenced this issue Mar 17, 2024
@lunny @wxiaoguang
Fix codeowner detected diff base branch to mergebase (#29783) (#29807)
85f31eb 
Fix #29763
Backport #29783 

This PR fixes 2 problems with CodeOwner in the pull request.
- Don't use the pull request base branch but merge-base as a diff base
to detect the code owner.
- CodeOwner detection in fork repositories will be disabled because
almost all the fork repositories will not change CODEOWNERS files but it
should not be used on fork repositories' pull requests.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
type/bug
Projects
None yet
Milestone
1.21.9
Development

Successfully merging a pull request may close this issue.

Fix codeowner detected diff base branch to mergebase lunny/gitea
2 participants
@lunny @baronjeppe