From 55e0756c68057eedfdb2f6ce7e9d8f9a5b228150 Mon Sep 17 00:00:00 2001 From: Chai-Shi Date: Sat, 18 Jan 2025 03:37:17 +0800 Subject: [PATCH] Fix push message behavior (#33215) Fixes #32769 by the logic from pr #33192 --------- Co-authored-by: wxiaoguang --- models/git/branch.go | 4 + routers/web/repo/view_home.go | 20 ++- services/repository/branch.go | 57 +++++++++ services/repository/merge_upstream.go | 67 +--------- .../repo/code/upstream_diverging_info.tmpl | 6 +- tests/integration/repo_branch_test.go | 116 ++++++++---------- 6 files changed, 138 insertions(+), 132 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index e683ce47e6..a264f555d9 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -440,6 +440,8 @@ type FindRecentlyPushedNewBranchesOptions struct { } type RecentlyPushedNewBranch struct { + BranchRepo *repo_model.Repository + BranchName string BranchDisplayName string BranchLink string BranchCompareURL string @@ -540,7 +542,9 @@ func FindRecentlyPushedNewBranches(ctx context.Context, doer *user_model.User, o branchDisplayName = fmt.Sprintf("%s:%s", branch.Repo.FullName(), branchDisplayName) } newBranches = append(newBranches, &RecentlyPushedNewBranch{ + BranchRepo: branch.Repo, BranchDisplayName: branchDisplayName, + BranchName: branch.Name, 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, diff --git a/routers/web/repo/view_home.go b/routers/web/repo/view_home.go index ff2ee3c7ec..d141df181c 100644 --- a/routers/web/repo/view_home.go +++ b/routers/web/repo/view_home.go @@ -215,10 +215,28 @@ func prepareRecentlyPushedNewBranches(ctx *context.Context) { if !opts.Repo.IsMirror && !opts.BaseRepo.IsMirror && opts.BaseRepo.UnitEnabled(ctx, unit_model.TypePullRequests) && baseRepoPerm.CanRead(unit_model.TypePullRequests) { - ctx.Data["RecentlyPushedNewBranches"], err = git_model.FindRecentlyPushedNewBranches(ctx, ctx.Doer, opts) + var finalBranches []*git_model.RecentlyPushedNewBranch + branches, err := git_model.FindRecentlyPushedNewBranches(ctx, ctx.Doer, opts) if err != nil { log.Error("FindRecentlyPushedNewBranches failed: %v", err) } + + for _, branch := range branches { + divergingInfo, err := repo_service.GetBranchDivergingInfo(ctx, + branch.BranchRepo, branch.BranchName, // "base" repo for diverging info + opts.BaseRepo, opts.BaseRepo.DefaultBranch, // "head" repo for diverging info + ) + if err != nil { + log.Error("GetBranchDivergingInfo failed: %v", err) + continue + } + branchRepoHasNewCommits := divergingInfo.BaseHasNewCommits + baseRepoCommitsBehind := divergingInfo.HeadCommitsBehind + if branchRepoHasNewCommits || baseRepoCommitsBehind > 0 { + finalBranches = append(finalBranches, branch) + } + } + ctx.Data["RecentlyPushedNewBranches"] = finalBranches } } } diff --git a/services/repository/branch.go b/services/repository/branch.go index a1065f5641..08c53bbb6a 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -26,6 +26,7 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/queue" repo_module "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/reqctx" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" @@ -665,3 +666,59 @@ func SetRepoDefaultBranch(ctx context.Context, repo *repo_model.Repository, gitR return nil } + +// BranchDivergingInfo contains the information about the divergence of a head branch to the base branch. +// This struct is also used in templates, so it needs to search for all references before changing it. +type BranchDivergingInfo struct { + BaseHasNewCommits bool + HeadCommitsBehind int + HeadCommitsAhead int +} + +// GetBranchDivergingInfo returns the information about the divergence of a patch branch to the base branch. +func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repository, baseBranch string, headRepo *repo_model.Repository, headBranch string) (*BranchDivergingInfo, error) { + headGitBranch, err := git_model.GetBranch(ctx, headRepo.ID, headBranch) + if err != nil { + return nil, err + } + + baseGitBranch, err := git_model.GetBranch(ctx, baseRepo.ID, baseBranch) + if err != nil { + return nil, err + } + + info := &BranchDivergingInfo{} + if headGitBranch.CommitID == baseGitBranch.CommitID { + return info, nil + } + + // if the fork repo has new commits, this call will fail because they are not in the base repo + // exit status 128 - fatal: Invalid symmetric difference expression aaaaaaaaaaaa...bbbbbbbbbbbb + // so at the moment, we first check the update time, then check whether the fork branch has base's head + diff, err := git.GetDivergingCommits(ctx, baseRepo.RepoPath(), baseGitBranch.CommitID, headGitBranch.CommitID) + if err != nil { + info.BaseHasNewCommits = baseGitBranch.UpdatedUnix > headGitBranch.UpdatedUnix + if headRepo.IsFork && info.BaseHasNewCommits { + return info, nil + } + // if the base's update time is before the fork, check whether the base's head is in the fork + headGitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, headRepo) + if err != nil { + return nil, err + } + headCommit, err := headGitRepo.GetCommit(headGitBranch.CommitID) + if err != nil { + return nil, err + } + baseCommitID, err := git.NewIDFromString(baseGitBranch.CommitID) + if err != nil { + return nil, err + } + hasPreviousCommit, _ := headCommit.HasPreviousCommit(baseCommitID) + info.BaseHasNewCommits = !hasPreviousCommit + return info, nil + } + + info.HeadCommitsBehind, info.HeadCommitsAhead = diff.Behind, diff.Ahead + return info, nil +} diff --git a/services/repository/merge_upstream.go b/services/repository/merge_upstream.go index ef161889c0..008e7de385 100644 --- a/services/repository/merge_upstream.go +++ b/services/repository/merge_upstream.go @@ -7,24 +7,16 @@ import ( "context" "fmt" - git_model "code.gitea.io/gitea/models/git" issue_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/gitrepo" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/reqctx" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/pull" ) -type UpstreamDivergingInfo struct { - BaseHasNewCommits bool - CommitsBehind int - CommitsAhead int -} - // MergeUpstream merges the base repository's default branch into the fork repository's current branch. func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branch string) (mergeStyle string, err error) { if err = repo.MustNotBeArchived(); err != nil { @@ -78,67 +70,18 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model. } // GetUpstreamDivergingInfo returns the information about the divergence between the fork repository's branch and the base repository's default branch. -func GetUpstreamDivergingInfo(ctx reqctx.RequestContext, repo *repo_model.Repository, branch string) (*UpstreamDivergingInfo, error) { - if !repo.IsFork { +func GetUpstreamDivergingInfo(ctx reqctx.RequestContext, forkRepo *repo_model.Repository, forkBranch string) (*BranchDivergingInfo, error) { + if !forkRepo.IsFork { return nil, util.NewInvalidArgumentErrorf("repo is not a fork") } - if repo.IsArchived { + if forkRepo.IsArchived { return nil, util.NewInvalidArgumentErrorf("repo is archived") } - if err := repo.GetBaseRepo(ctx); err != nil { + if err := forkRepo.GetBaseRepo(ctx); err != nil { return nil, err } - forkBranch, err := git_model.GetBranch(ctx, repo.ID, branch) - if err != nil { - return nil, err - } - - baseBranch, err := git_model.GetBranch(ctx, repo.BaseRepo.ID, repo.BaseRepo.DefaultBranch) - if err != nil { - return nil, err - } - - info := &UpstreamDivergingInfo{} - if forkBranch.CommitID == baseBranch.CommitID { - return info, nil - } - - // if the fork repo has new commits, this call will fail because they are not in the base repo - // exit status 128 - fatal: Invalid symmetric difference expression aaaaaaaaaaaa...bbbbbbbbbbbb - // so at the moment, we first check the update time, then check whether the fork branch has base's head - diff, err := git.GetDivergingCommits(ctx, repo.BaseRepo.RepoPath(), baseBranch.CommitID, forkBranch.CommitID) - if err != nil { - info.BaseHasNewCommits = baseBranch.UpdatedUnix > forkBranch.UpdatedUnix - if info.BaseHasNewCommits { - return info, nil - } - - // if the base's update time is before the fork, check whether the base's head is in the fork - baseGitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo.BaseRepo) - if err != nil { - return nil, err - } - headGitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo) - if err != nil { - return nil, err - } - - baseCommitID, err := baseGitRepo.ConvertToGitID(baseBranch.CommitID) - if err != nil { - return nil, err - } - headCommit, err := headGitRepo.GetCommit(forkBranch.CommitID) - if err != nil { - return nil, err - } - hasPreviousCommit, _ := headCommit.HasPreviousCommit(baseCommitID) - info.BaseHasNewCommits = !hasPreviousCommit - return info, nil - } - - info.CommitsBehind, info.CommitsAhead = diff.Behind, diff.Ahead - return info, nil + return GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkRepo.BaseRepo.DefaultBranch, forkRepo, forkBranch) } diff --git a/templates/repo/code/upstream_diverging_info.tmpl b/templates/repo/code/upstream_diverging_info.tmpl index a1f37b8c05..8d6e55959f 100644 --- a/templates/repo/code/upstream_diverging_info.tmpl +++ b/templates/repo/code/upstream_diverging_info.tmpl @@ -1,10 +1,10 @@ -{{if and .UpstreamDivergingInfo (or .UpstreamDivergingInfo.BaseHasNewCommits .UpstreamDivergingInfo.CommitsBehind)}} +{{if and .UpstreamDivergingInfo (or .UpstreamDivergingInfo.BaseHasNewCommits .UpstreamDivergingInfo.HeadCommitsBehind)}}
{{$upstreamLink := printf "%s/src/branch/%s" .Repository.BaseRepo.Link (.Repository.BaseRepo.DefaultBranch|PathEscapeSegments)}} {{$upstreamHtml := HTMLFormat `%s:%s` $upstreamLink .Repository.BaseRepo.FullName .Repository.BaseRepo.DefaultBranch}} - {{if .UpstreamDivergingInfo.CommitsBehind}} - {{ctx.Locale.TrN .UpstreamDivergingInfo.CommitsBehind "repo.pulls.upstream_diverging_prompt_behind_1" "repo.pulls.upstream_diverging_prompt_behind_n" .UpstreamDivergingInfo.CommitsBehind $upstreamHtml}} + {{if .UpstreamDivergingInfo.HeadCommitsBehind}} + {{ctx.Locale.TrN .UpstreamDivergingInfo.HeadCommitsBehind "repo.pulls.upstream_diverging_prompt_behind_1" "repo.pulls.upstream_diverging_prompt_behind_n" .UpstreamDivergingInfo.HeadCommitsBehind $upstreamHtml}} {{else}} {{ctx.Locale.Tr "repo.pulls.upstream_diverging_prompt_base_newer" $upstreamHtml}} {{end}} diff --git a/tests/integration/repo_branch_test.go b/tests/integration/repo_branch_test.go index 2b4c417334..f9cf13112a 100644 --- a/tests/integration/repo_branch_test.go +++ b/tests/integration/repo_branch_test.go @@ -11,13 +11,8 @@ import ( "strings" "testing" - auth_model "code.gitea.io/gitea/models/auth" - org_model "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" - api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/tests" @@ -142,19 +137,51 @@ func TestCreateBranchInvalidCSRF(t *testing.T) { assert.Contains(t, resp.Body.String(), "Invalid CSRF token") } -func prepareBranch(t *testing.T, session *TestSession, repo *repo_model.Repository) { - baseRefSubURL := fmt.Sprintf("branch/%s", repo.DefaultBranch) - +func prepareRecentlyPushedBranchTest(t *testing.T, headSession *TestSession, baseRepo, headRepo *repo_model.Repository) { + refSubURL := fmt.Sprintf("branch/%s", headRepo.DefaultBranch) + baseRepoPath := baseRepo.OwnerName + "/" + baseRepo.Name + headRepoPath := headRepo.OwnerName + "/" + headRepo.Name + // Case 1: Normal branch changeset to display pushed message // create branch with no new commit - testCreateBranch(t, session, repo.OwnerName, repo.Name, baseRefSubURL, "no-commit", http.StatusSeeOther) + testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, refSubURL, "no-commit", http.StatusSeeOther) // create branch with commit - testCreateBranch(t, session, repo.OwnerName, repo.Name, baseRefSubURL, "new-commit", http.StatusSeeOther) - testAPINewFile(t, session, repo.OwnerName, repo.Name, "new-commit", "new-commit.txt", "new-commit") + testAPINewFile(t, headSession, headRepo.OwnerName, headRepo.Name, "new-commit", fmt.Sprintf("new-file-%s.txt", headRepo.Name), "new-commit") - // create deleted branch - testCreateBranch(t, session, repo.OwnerName, repo.Name, "branch/new-commit", "deleted-branch", http.StatusSeeOther) - testUIDeleteBranch(t, session, repo.OwnerName, repo.Name, "deleted-branch") + // create a branch then delete it + testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "deleted-branch", http.StatusSeeOther) + testUIDeleteBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "deleted-branch") + + // only `new-commit` branch has commits ahead the base branch + checkRecentlyPushedNewBranches(t, headSession, headRepoPath, []string{"new-commit"}) + if baseRepo.RepoPath() != headRepo.RepoPath() { + checkRecentlyPushedNewBranches(t, headSession, baseRepoPath, []string{fmt.Sprintf("%v:new-commit", headRepo.FullName())}) + } + + // Case 2: Create PR so that `new-commit` branch will not show + testCreatePullToDefaultBranch(t, headSession, baseRepo, headRepo, "new-commit", "merge new-commit to default branch") + // No push message show because of active PR + checkRecentlyPushedNewBranches(t, headSession, headRepoPath, []string{}) + if baseRepo.RepoPath() != headRepo.RepoPath() { + checkRecentlyPushedNewBranches(t, headSession, baseRepoPath, []string{}) + } +} + +func prepareRecentlyPushedBranchSpecialTest(t *testing.T, session *TestSession, baseRepo, headRepo *repo_model.Repository) { + refSubURL := fmt.Sprintf("branch/%s", headRepo.DefaultBranch) + baseRepoPath := baseRepo.OwnerName + "/" + baseRepo.Name + headRepoPath := headRepo.OwnerName + "/" + headRepo.Name + // create branch with no new commit + testCreateBranch(t, session, headRepo.OwnerName, headRepo.Name, refSubURL, "no-commit-special", http.StatusSeeOther) + + // update base (default) branch before head branch is updated + testAPINewFile(t, session, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, fmt.Sprintf("new-file-special-%s.txt", headRepo.Name), "new-commit") + + // Though we have new `no-commit` branch, but the headBranch is not newer or commits ahead baseBranch. No message show. + checkRecentlyPushedNewBranches(t, session, headRepoPath, []string{}) + if baseRepo.RepoPath() != headRepo.RepoPath() { + checkRecentlyPushedNewBranches(t, session, baseRepoPath, []string{}) + } } func testCreatePullToDefaultBranch(t *testing.T, session *TestSession, baseRepo, headRepo *repo_model.Repository, headBranch, title string) string { @@ -169,6 +196,9 @@ func testCreatePullToDefaultBranch(t *testing.T, session *TestSession, baseRepo, } func prepareRepoPR(t *testing.T, baseSession, headSession *TestSession, baseRepo, headRepo *repo_model.Repository) { + refSubURL := fmt.Sprintf("branch/%s", headRepo.DefaultBranch) + testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, refSubURL, "new-commit", http.StatusSeeOther) + // create opening PR testCreateBranch(t, headSession, headRepo.OwnerName, headRepo.Name, "branch/new-commit", "opening-pr", http.StatusSeeOther) testCreatePullToDefaultBranch(t, baseSession, baseRepo, headRepo, "opening-pr", "opening pr") @@ -210,65 +240,19 @@ func checkRecentlyPushedNewBranches(t *testing.T, session *TestSession, repoPath func TestRecentlyPushedNewBranches(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { - user1Session := loginUser(t, "user1") - user2Session := loginUser(t, "user2") user12Session := loginUser(t, "user12") - user13Session := loginUser(t, "user13") - // prepare branch and PRs in original repo + // Same reposioty check repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - prepareBranch(t, user12Session, repo10) prepareRepoPR(t, user12Session, user12Session, repo10, repo10) - - // outdated new branch should not be displayed - checkRecentlyPushedNewBranches(t, user12Session, "user12/repo10", []string{"new-commit"}) + prepareRecentlyPushedBranchTest(t, user12Session, repo10, repo10) + prepareRecentlyPushedBranchSpecialTest(t, user12Session, repo10, repo10) // create a fork repo in public org - testRepoFork(t, user12Session, repo10.OwnerName, repo10.Name, "org25", "org25_fork_repo10", "new-commit") + testRepoFork(t, user12Session, repo10.OwnerName, repo10.Name, "org25", "org25_fork_repo10", repo10.DefaultBranch) orgPublicForkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: 25, Name: "org25_fork_repo10"}) prepareRepoPR(t, user12Session, user12Session, repo10, orgPublicForkRepo) - - // user12 is the owner of the repo10 and the organization org25 - // in repo10, user12 has opening/closed/merged pr and closed/merged pr with deleted branch - checkRecentlyPushedNewBranches(t, user12Session, "user12/repo10", []string{"org25/org25_fork_repo10:new-commit", "new-commit"}) - - userForkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) - testCtx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddUser13AsCollaborator", doAPIAddCollaborator(testCtx, "user13", perm.AccessModeWrite)) - prepareBranch(t, user13Session, userForkRepo) - prepareRepoPR(t, user13Session, user13Session, repo10, userForkRepo) - - // create branch with same name in different repo by user13 - testCreateBranch(t, user13Session, repo10.OwnerName, repo10.Name, "branch/new-commit", "same-name-branch", http.StatusSeeOther) - testCreateBranch(t, user13Session, userForkRepo.OwnerName, userForkRepo.Name, "branch/new-commit", "same-name-branch", http.StatusSeeOther) - testCreatePullToDefaultBranch(t, user13Session, repo10, userForkRepo, "same-name-branch", "same name branch pr") - - // user13 pushed 2 branches with the same name in repo10 and repo11 - // and repo11's branch has a pr, but repo10's branch doesn't - // in this case, we should get repo10's branch but not repo11's branch - checkRecentlyPushedNewBranches(t, user13Session, "user12/repo10", []string{"same-name-branch", "user13/repo11:new-commit"}) - - // create a fork repo in private org - testRepoFork(t, user1Session, repo10.OwnerName, repo10.Name, "private_org35", "org35_fork_repo10", "new-commit") - orgPrivateForkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: 35, Name: "org35_fork_repo10"}) - prepareRepoPR(t, user1Session, user1Session, repo10, orgPrivateForkRepo) - - // user1 is the owner of private_org35 and no write permission to repo10 - // so user1 can only see the branch in org35_fork_repo10 - checkRecentlyPushedNewBranches(t, user1Session, "user12/repo10", []string{"private_org35/org35_fork_repo10:new-commit"}) - - // user2 push a branch in private_org35 - testCreateBranch(t, user2Session, orgPrivateForkRepo.OwnerName, orgPrivateForkRepo.Name, "branch/new-commit", "user-read-permission", http.StatusSeeOther) - // convert write permission to read permission for code unit - token := getTokenForLoggedInUser(t, user1Session, auth_model.AccessTokenScopeWriteOrganization) - req := NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/teams/%d", 24), &api.EditTeamOption{ - Name: "team24", - UnitsMap: map[string]string{"repo.code": "read"}, - }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusOK) - teamUnit := unittest.AssertExistsAndLoadBean(t, &org_model.TeamUnit{TeamID: 24, Type: unit.TypeCode}) - assert.Equal(t, perm.AccessModeRead, teamUnit.AccessMode) - // user2 can see the branch as it is created by user2 - checkRecentlyPushedNewBranches(t, user2Session, "user12/repo10", []string{"private_org35/org35_fork_repo10:user-read-permission"}) + prepareRecentlyPushedBranchTest(t, user12Session, repo10, orgPublicForkRepo) + prepareRecentlyPushedBranchSpecialTest(t, user12Session, repo10, orgPublicForkRepo) }) }