Skip to content

Commit

Permalink
Fix wrong display of recently pushed notification (go-gitea#25812)
Browse files Browse the repository at this point in the history
There's a bug in go-gitea#25715:
If user pushed a commit into another repo with same branch name, the
no-related repo will display the recently pushed notification
incorrectly.
It is simple to fix this, we should match the repo id in the sql query.

![image](https://github.com/go-gitea/gitea/assets/18380374/9411a926-16f1-419e-a1b5-e953af38bab1)
The latest commit is 2 weeks ago.

![image](https://github.com/go-gitea/gitea/assets/18380374/52f9ab22-4999-43ac-a86f-6d36fb1e0411)

The notification comes from another repo with same branch name:

![image](https://github.com/go-gitea/gitea/assets/18380374/a26bc335-8e5b-4b9c-a965-c3dc3fa6f252)

After:
In forked repo:

![image](https://github.com/go-gitea/gitea/assets/18380374/ce6ffc35-deb7-4be7-8b09-184207392f32)
New PR Link will redirect to the original repo:

![image](https://github.com/go-gitea/gitea/assets/18380374/7b98e76f-0c75-494c-9462-80cf9f98e786)
In the original repo:

![image](https://github.com/go-gitea/gitea/assets/18380374/5f6a821b-e51a-4bbd-9980-d9eb94a3c847)
New PR Link:

![image](https://github.com/go-gitea/gitea/assets/18380374/1ce8c879-9f11-4312-8c32-695d7d9af0df)

In the same repo:

![image](https://github.com/go-gitea/gitea/assets/18380374/64b56073-4d0e-40c4-b8a0-80be7a775f69)
New PR Link:

![image](https://github.com/go-gitea/gitea/assets/18380374/96e1b6a3-fb98-40ee-b2ee-648039fb0dcf)

08/15 Update:
Follow go-gitea#26257, added permission check and logic fix mentioned in
go-gitea#26257 (comment)

2024/04/25 Update:
Fix go-gitea#30611

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
  • Loading branch information
4 people committed May 22, 2024
1 parent e3390e2 commit a940669
Show file tree
Hide file tree
Showing 26 changed files with 506 additions and 70 deletions.
36 changes: 36 additions & 0 deletions models/fixtures/branch.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,39 @@
is_deleted: false
deleted_by_id: 0
deleted_unix: 0

-
id: 5
repo_id: 10
name: 'master'
commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d'
commit_message: 'Initial commit'
commit_time: 1489927679
pusher_id: 12
is_deleted: false
deleted_by_id: 0
deleted_unix: 0

-
id: 6
repo_id: 10
name: 'outdated-new-branch'
commit_id: 'cb24c347e328d83c1e0c3c908a6b2c0a2fcb8a3d'
commit_message: 'add'
commit_time: 1489927679
pusher_id: 12
is_deleted: false
deleted_by_id: 0
deleted_unix: 0

-
id: 14
repo_id: 11
name: 'master'
commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d'
commit_message: 'Initial commit'
commit_time: 1489927679
pusher_id: 13
is_deleted: false
deleted_by_id: 0
deleted_unix: 0
8 changes: 8 additions & 0 deletions models/fixtures/issue_index.yml
Original file line number Diff line number Diff line change
@@ -1,27 +1,35 @@
-
group_id: 1
max_index: 5

-
group_id: 2
max_index: 2

-
group_id: 3
max_index: 2

-
group_id: 10
max_index: 1

-
group_id: 32
max_index: 2

-
group_id: 48
max_index: 1

-
group_id: 42
max_index: 1

-
group_id: 50
max_index: 1

-
group_id: 51
max_index: 1
12 changes: 12 additions & 0 deletions models/fixtures/org_user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,15 @@
uid: 40
org_id: 41
is_public: true

-
id: 21
uid: 12
org_id: 25
is_public: true

-
id: 22
uid: 2
org_id: 35
is_public: true
2 changes: 1 addition & 1 deletion models/fixtures/repository.yml
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@
is_archived: false
is_mirror: false
status: 0
is_fork: false
is_fork: true
fork_id: 10
is_template: false
template_id: 0
Expand Down
22 changes: 22 additions & 0 deletions models/fixtures/team.yml
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,25 @@
num_members: 2
includes_all_repositories: false
can_create_org_repo: false

-
id: 23
org_id: 25
lower_name: owners
name: Owners
authorize: 4 # owner
num_repos: 0
num_members: 1
includes_all_repositories: false
can_create_org_repo: true

-
id: 24
org_id: 35
lower_name: team24
name: team24
authorize: 2 # write
num_repos: 0
num_members: 1
includes_all_repositories: true
can_create_org_repo: false
18 changes: 18 additions & 0 deletions models/fixtures/team_unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,21 @@
team_id: 22
type: 3
access_mode: 1

-
id: 55
team_id: 18
type: 1 # code
access_mode: 4

-
id: 56
team_id: 23
type: 1 # code
access_mode: 4

-
id: 57
team_id: 24
type: 1 # code
access_mode: 2
12 changes: 12 additions & 0 deletions models/fixtures/team_user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,15 @@
org_id: 41
team_id: 22
uid: 39

-
id: 26
org_id: 25
team_id: 23
uid: 12

-
id: 27
org_id: 35
team_id: 24
uid: 2
8 changes: 4 additions & 4 deletions models/fixtures/user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -918,8 +918,8 @@
num_following: 0
num_stars: 0
num_repos: 0
num_teams: 1
num_members: 1
num_teams: 2
num_members: 2
visibility: 0
repo_admin_change_team_access: false
theme: ""
Expand Down Expand Up @@ -1289,8 +1289,8 @@
num_following: 0
num_stars: 0
num_repos: 0
num_teams: 1
num_members: 1
num_teams: 2
num_members: 2
visibility: 2
repo_admin_change_team_access: false
theme: ""
Expand Down
142 changes: 120 additions & 22 deletions models/git/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ import (

"code.gitea.io/gitea/models/db"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"

Expand Down Expand Up @@ -102,8 +104,9 @@ func (err ErrBranchesEqual) Unwrap() error {
// for pagination, keyword search and filtering
type Branch struct {
ID int64
RepoID int64 `xorm:"UNIQUE(s)"`
Name string `xorm:"UNIQUE(s) NOT NULL"` // git's ref-name is case-sensitive internally, however, in some databases (mssql, mysql, by default), it's case-insensitive at the moment
RepoID int64 `xorm:"UNIQUE(s)"`
Repo *repo_model.Repository `xorm:"-"`
Name string `xorm:"UNIQUE(s) NOT NULL"` // git's ref-name is case-sensitive internally, however, in some databases (mssql, mysql, by default), it's case-insensitive at the moment
CommitID string
CommitMessage string `xorm:"TEXT"` // it only stores the message summary (the first line)
PusherID int64
Expand Down Expand Up @@ -139,6 +142,14 @@ func (b *Branch) LoadPusher(ctx context.Context) (err error) {
return err
}

func (b *Branch) LoadRepo(ctx context.Context) (err error) {
if b.Repo != nil || b.RepoID == 0 {
return nil
}
b.Repo, err = repo_model.GetRepositoryByID(ctx, b.RepoID)
return err
}

func init() {
db.RegisterModel(new(Branch))
db.RegisterModel(new(RenamedBranch))
Expand Down Expand Up @@ -400,24 +411,111 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str
return committer.Commit()
}

// FindRecentlyPushedNewBranches return at most 2 new branches pushed by the user in 6 hours which has no opened PRs created
// except the indicate branch
func FindRecentlyPushedNewBranches(ctx context.Context, repoID, userID int64, excludeBranchName string) (BranchList, error) {
branches := make(BranchList, 0, 2)
subQuery := builder.Select("head_branch").From("pull_request").
InnerJoin("issue", "issue.id = pull_request.issue_id").
Where(builder.Eq{
"pull_request.head_repo_id": repoID,
"issue.is_closed": false,
})
err := db.GetEngine(ctx).
Where("pusher_id=? AND is_deleted=?", userID, false).
And("name <> ?", excludeBranchName).
And("repo_id = ?", repoID).
And("commit_time >= ?", time.Now().Add(-time.Hour*6).Unix()).
NotIn("name", subQuery).
OrderBy("branch.commit_time DESC").
Limit(2).
Find(&branches)
return branches, err
type FindRecentlyPushedNewBranchesOptions struct {
Repo *repo_model.Repository
BaseRepo *repo_model.Repository
CommitAfterUnix int64
MaxCount int
}

type RecentlyPushedNewBranch struct {
BranchDisplayName string
BranchLink string
BranchCompareURL string
CommitTime timeutil.TimeStamp
}

// FindRecentlyPushedNewBranches return at most 2 new branches pushed by the user in 2 hours which has no opened PRs created
// if opts.CommitAfterUnix is 0, we will find the branches that were committed to in the last 2 hours
// if opts.ListOptions is not set, we will only display top 2 latest branch
func FindRecentlyPushedNewBranches(ctx context.Context, doer *user_model.User, opts *FindRecentlyPushedNewBranchesOptions) ([]*RecentlyPushedNewBranch, error) {
if doer == nil {
return []*RecentlyPushedNewBranch{}, nil
}

// find all related repo ids
repoOpts := repo_model.SearchRepoOptions{
Actor: doer,
Private: true,
AllPublic: false, // Include also all public repositories of users and public organisations
AllLimited: false, // Include also all public repositories of limited organisations
Fork: optional.Some(true),
ForkFrom: opts.BaseRepo.ID,
Archived: optional.Some(false),
}
repoCond := repo_model.SearchRepositoryCondition(&repoOpts).And(repo_model.AccessibleRepositoryCondition(doer, unit.TypeCode))
if opts.Repo.ID == opts.BaseRepo.ID {
// should also include the base repo's branches
repoCond = repoCond.Or(builder.Eq{"id": opts.BaseRepo.ID})
} else {
// in fork repo, we only detect the fork repo's branch
repoCond = repoCond.And(builder.Eq{"id": opts.Repo.ID})
}
repoIDs := builder.Select("id").From("repository").Where(repoCond)

if opts.CommitAfterUnix == 0 {
opts.CommitAfterUnix = time.Now().Add(-time.Hour * 2).Unix()
}

baseBranch, err := GetBranch(ctx, opts.BaseRepo.ID, opts.BaseRepo.DefaultBranch)
if err != nil {
return nil, err
}

// find all related branches, these branches may already created PRs, we will check later
var branches []*Branch
if err := db.GetEngine(ctx).
Where(builder.And(
builder.Eq{
"pusher_id": doer.ID,
"is_deleted": false,
},
builder.Gte{"commit_time": opts.CommitAfterUnix},
builder.In("repo_id", repoIDs),
// newly created branch have no changes, so skip them
builder.Neq{"commit_id": baseBranch.CommitID},
)).
OrderBy(db.SearchOrderByRecentUpdated.String()).
Find(&branches); err != nil {
return nil, err
}

newBranches := make([]*RecentlyPushedNewBranch, 0, len(branches))
if opts.MaxCount == 0 {
// by default we display 2 recently pushed new branch
opts.MaxCount = 2
}
for _, branch := range branches {
// whether branch have already created PR
count, err := db.GetEngine(ctx).Table("pull_request").
// we should not only use branch name here, because if there are branches with same name in other repos,
// we can not detect them correctly
Where(builder.Eq{"head_repo_id": branch.RepoID, "head_branch": branch.Name}).Count()
if err != nil {
return nil, err
}

// if no PR, we add to the result
if count == 0 {
if err := branch.LoadRepo(ctx); err != nil {
return nil, err
}

branchDisplayName := branch.Name
if branch.Repo.ID != opts.BaseRepo.ID && branch.Repo.ID != opts.Repo.ID {
branchDisplayName = fmt.Sprintf("%s:%s", branch.Repo.FullName(), branchDisplayName)
}
newBranches = append(newBranches, &RecentlyPushedNewBranch{
BranchDisplayName: branchDisplayName,
BranchLink: fmt.Sprintf("%s/src/branch/%s", branch.Repo.Link(), util.PathEscapeSegments(branch.Name)),
BranchCompareURL: branch.Repo.ComposeBranchCompareURL(opts.BaseRepo, branch.Name),
CommitTime: branch.CommitTime,
})
}
if len(newBranches) == opts.MaxCount {
break
}
}

return newBranches, nil
}
19 changes: 19 additions & 0 deletions models/git/branch_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"

"code.gitea.io/gitea/models/db"
repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/optional"
Expand Down Expand Up @@ -59,6 +60,24 @@ func (branches BranchList) LoadPusher(ctx context.Context) error {
return nil
}

func (branches BranchList) LoadRepo(ctx context.Context) error {
ids := container.FilterSlice(branches, func(branch *Branch) (int64, bool) {
return branch.RepoID, branch.RepoID > 0 && branch.Repo == nil
})

reposMap := make(map[int64]*repo_model.Repository, len(ids))
if err := db.GetEngine(ctx).In("id", ids).Find(&reposMap); err != nil {
return err
}
for _, branch := range branches {
if branch.RepoID <= 0 || branch.Repo != nil {
continue
}
branch.Repo = reposMap[branch.RepoID]
}
return nil
}

type FindBranchOptions struct {
db.ListOptions
RepoID int64
Expand Down
Loading

0 comments on commit a940669

Please sign in to comment.