Skip to content

Commit db9fe86

Browse files
wxiaoguangwxiaoguang
committed
Improve sync fork behavior (go-gitea#33319)
Fix go-gitea#33271 Suppose there is a `branch-a` in fork repo: 1. if `branch-a` exists in base repo: try to sync `base:branch-a` to `fork:branch-a` 2. if `branch-a` doesn't exist in base repo: try to sync `base:main` to `fork:branch-a` # Conflicts: # services/repository/merge_upstream.go
1 parent e72d001 commit db9fe86

File tree

6 files changed

+108
-25
lines changed

6 files changed

+108
-25
lines changed

‎models/git/branch.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,9 @@ func GetBranch(ctx context.Context, repoID int64, branchName string) (*Branch, e
167167
BranchName: branchName,
168168
}
169169
}
170+
// FIXME: this design is not right: it doesn't check `branch.IsDeleted`, it doesn't make sense to make callers to check IsDeleted again and again.
171+
// It causes inconsistency with `GetBranches` and `git.GetBranch`, and will lead to strange bugs
172+
// In the future, there should be 2 functions: `GetBranchExisting` and `GetBranchWithDeleted`
170173
return &branch, nil
171174
}
172175

‎options/locale/locale_en-US.ini

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1951,7 +1951,7 @@ pulls.upstream_diverging_prompt_behind_1 = This branch is %[1]d commit behind %[
19511951
pulls.upstream_diverging_prompt_behind_n = This branch is %[1]d commits behind %[2]s
19521952
pulls.upstream_diverging_prompt_base_newer = The base branch %s has new changes
19531953
pulls.upstream_diverging_merge = Sync fork
1954-
pulls.upstream_diverging_merge_confirm = Would you like to merge base repository's default branch onto this repository's branch %s?
1954+
pulls.upstream_diverging_merge_confirm = Would you like to merge "%[1]s" onto "%[2]s"?
19551955

19561956
pull.deleted_branch = (deleted):%s
19571957
pull.agit_documentation = Review documentation about AGit

‎services/repository/branch.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -638,9 +638,12 @@ func SetRepoDefaultBranch(ctx context.Context, repo *repo_model.Repository, gitR
638638
}
639639

640640
// BranchDivergingInfo contains the information about the divergence of a head branch to the base branch.
641-
// This struct is also used in templates, so it needs to search for all references before changing it.
642641
type BranchDivergingInfo struct {
642+
// whether the base branch contains new commits which are not in the head branch
643643
BaseHasNewCommits bool
644+
645+
// behind/after are number of commits that the head branch is behind/after the base branch, it's 0 if it's unable to calculate.
646+
// there could be a case that BaseHasNewCommits=true while the behind/after are both 0 (unable to calculate).
644647
HeadCommitsBehind int
645648
HeadCommitsAhead int
646649
}
@@ -651,11 +654,20 @@ func GetBranchDivergingInfo(ctx context.Context, baseRepo *repo_model.Repository
651654
if err != nil {
652655
return nil, err
653656
}
654-
657+
if headGitBranch.IsDeleted {
658+
return nil, git_model.ErrBranchNotExist{
659+
BranchName: headBranch,
660+
}
661+
}
655662
baseGitBranch, err := git_model.GetBranch(ctx, baseRepo.ID, baseBranch)
656663
if err != nil {
657664
return nil, err
658665
}
666+
if baseGitBranch.IsDeleted {
667+
return nil, git_model.ErrBranchNotExist{
668+
BranchName: baseBranch,
669+
}
670+
}
659671

660672
info := &BranchDivergingInfo{}
661673
if headGitBranch.CommitID == baseGitBranch.CommitID {
@@ -692,5 +704,6 @@ func GetBranchDivergingInfo(ctx context.Context, baseRepo *repo_model.Repository
692704
}
693705

694706
info.HeadCommitsBehind, info.HeadCommitsAhead = diff.Behind, diff.Ahead
707+
info.BaseHasNewCommits = info.HeadCommitsBehind > 0
695708
return info, nil
696709
}

‎services/repository/merge_upstream.go

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
package repository
55

66
import (
7-
"context"
7+
"errors"
88
"fmt"
99

1010
issue_model "code.gitea.io/gitea/models/issues"
@@ -17,16 +17,24 @@ import (
1717
)
1818

1919
// MergeUpstream merges the base repository's default branch into the fork repository's current branch.
20-
func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branch string) (mergeStyle string, err error) {
20+
func MergeUpstream(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_model.Repository, branch string) (mergeStyle string, err error) {
2121
if err = repo.MustNotBeArchived(); err != nil {
2222
return "", err
2323
}
2424
if err = repo.GetBaseRepo(ctx); err != nil {
2525
return "", err
2626
}
27+
divergingInfo, err := GetUpstreamDivergingInfo(ctx, repo, branch)
28+
if err != nil {
29+
return "", err
30+
}
31+
if !divergingInfo.BaseBranchHasNewCommits {
32+
return "up-to-date", nil
33+
}
34+
2735
err = git.Push(ctx, repo.BaseRepo.RepoPath(), git.PushOptions{
2836
Remote: repo.RepoPath(),
29-
Branch: fmt.Sprintf("%s:%s", repo.BaseRepo.DefaultBranch, branch),
37+
Branch: fmt.Sprintf("%s:%s", divergingInfo.BaseBranchName, branch),
3038
Env: repo_module.PushingEnvironment(doer, repo),
3139
})
3240
if err == nil {
@@ -58,7 +66,7 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.
5866
BaseRepoID: repo.BaseRepo.ID,
5967
BaseRepo: repo.BaseRepo,
6068
HeadBranch: branch, // maybe HeadCommitID is not needed
61-
BaseBranch: repo.BaseRepo.DefaultBranch,
69+
BaseBranch: divergingInfo.BaseBranchName,
6270
}
6371
fakeIssue.PullRequest = fakePR
6472
err = pull.Update(ctx, fakePR, doer, "merge upstream", false)
@@ -68,6 +76,13 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.
6876
return "merge", nil
6977
}
7078

79+
// UpstreamDivergingInfo is also used in templates, so it needs to search for all references before changing it.
80+
type UpstreamDivergingInfo struct {
81+
BaseBranchName string
82+
BaseBranchHasNewCommits bool
83+
HeadBranchCommitsBehind int
84+
}
85+
7186
// GetUpstreamDivergingInfo returns the information about the divergence between the fork repository's branch and the base repository's default branch.
7287
func GetUpstreamDivergingInfo(ctx context.Context, forkRepo *repo_model.Repository, forkBranch string) (*BranchDivergingInfo, error) {
7388
if !forkRepo.IsFork {
@@ -82,5 +97,26 @@ func GetUpstreamDivergingInfo(ctx context.Context, forkRepo *repo_model.Reposito
8297
return nil, err
8398
}
8499

85-
return GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkRepo.BaseRepo.DefaultBranch, forkRepo, forkBranch)
100+
// Do the best to follow the GitHub's behavior, suppose there is a `branch-a` in fork repo:
101+
// * if `branch-a` exists in base repo: try to sync `base:branch-a` to `fork:branch-a`
102+
// * if `branch-a` doesn't exist in base repo: try to sync `base:main` to `fork:branch-a`
103+
info, err := GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkBranch, forkRepo, forkBranch)
104+
if err == nil {
105+
return &UpstreamDivergingInfo{
106+
BaseBranchName: forkBranch,
107+
BaseBranchHasNewCommits: info.BaseHasNewCommits,
108+
HeadBranchCommitsBehind: info.HeadCommitsBehind,
109+
}, nil
110+
}
111+
if errors.Is(err, util.ErrNotExist) {
112+
info, err = GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkRepo.BaseRepo.DefaultBranch, forkRepo, forkBranch)
113+
if err == nil {
114+
return &UpstreamDivergingInfo{
115+
BaseBranchName: forkRepo.BaseRepo.DefaultBranch,
116+
BaseBranchHasNewCommits: info.BaseHasNewCommits,
117+
HeadBranchCommitsBehind: info.HeadCommitsBehind,
118+
}, nil
119+
}
120+
}
121+
return nil, err
86122
}

‎templates/repo/code/upstream_diverging_info.tmpl

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,20 @@
1-
{{if and .UpstreamDivergingInfo (or .UpstreamDivergingInfo.BaseHasNewCommits .UpstreamDivergingInfo.HeadCommitsBehind)}}
1+
{{if and .UpstreamDivergingInfo .UpstreamDivergingInfo.BaseBranchHasNewCommits}}
22
<div class="ui message flex-text-block">
33
<div class="tw-flex-1">
4-
{{$upstreamLink := printf "%s/src/branch/%s" .Repository.BaseRepo.Link (.Repository.BaseRepo.DefaultBranch|PathEscapeSegments)}}
5-
{{$upstreamHtml := HTMLFormat `<a href="%s">%s:%s</a>` $upstreamLink .Repository.BaseRepo.FullName .Repository.BaseRepo.DefaultBranch}}
6-
{{if .UpstreamDivergingInfo.HeadCommitsBehind}}
7-
{{ctx.Locale.TrN .UpstreamDivergingInfo.HeadCommitsBehind "repo.pulls.upstream_diverging_prompt_behind_1" "repo.pulls.upstream_diverging_prompt_behind_n" .UpstreamDivergingInfo.HeadCommitsBehind $upstreamHtml}}
4+
{{$upstreamLink := printf "%s/src/branch/%s" .Repository.BaseRepo.Link (.UpstreamDivergingInfo.BaseBranchName|PathEscapeSegments)}}
5+
{{$upstreamRepoBranchDisplay := HTMLFormat "%s:%s" .Repository.BaseRepo.FullName .UpstreamDivergingInfo.BaseBranchName}}
6+
{{$thisRepoBranchDisplay := HTMLFormat "%s:%s" .Repository.FullName .BranchName}}
7+
{{$upstreamHtml := HTMLFormat `<a href="%s">%s</a>` $upstreamLink $upstreamRepoBranchDisplay}}
8+
{{if .UpstreamDivergingInfo.HeadBranchCommitsBehind}}
9+
{{ctx.Locale.TrN .UpstreamDivergingInfo.HeadBranchCommitsBehind "repo.pulls.upstream_diverging_prompt_behind_1" "repo.pulls.upstream_diverging_prompt_behind_n" .UpstreamDivergingInfo.HeadBranchCommitsBehind $upstreamHtml}}
810
{{else}}
911
{{ctx.Locale.Tr "repo.pulls.upstream_diverging_prompt_base_newer" $upstreamHtml}}
1012
{{end}}
1113
</div>
1214
{{if .CanWriteCode}}
1315
<button class="ui compact primary button tw-m-0 link-action"
1416
data-modal-confirm-header="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge"}}"
15-
data-modal-confirm-content="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge_confirm" .BranchName}}"
17+
data-modal-confirm-content="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge_confirm" $upstreamRepoBranchDisplay $thisRepoBranchDisplay}}"
1618
data-url="{{.Repository.Link}}/branches/merge-upstream?branch={{.BranchName}}">
1719
{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge"}}
1820
</button>

‎tests/integration/repo_merge_upstream_test.go

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -60,25 +60,54 @@ func TestRepoMergeUpstream(t *testing.T) {
6060

6161
t.Run("HeadBeforeBase", func(t *testing.T) {
6262
// add a file in base repo
63+
sessionBaseUser := loginUser(t, baseUser.Name)
6364
require.NoError(t, createOrReplaceFileInBranch(baseUser, baseRepo, "new-file.txt", "master", "test-content-1"))
6465

65-
// the repo shows a prompt to "sync fork"
6666
var mergeUpstreamLink string
67-
require.Eventually(t, func() bool {
68-
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
69-
htmlDoc := NewHTMLParser(t, resp.Body)
70-
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
71-
if mergeUpstreamLink == "" {
72-
return false
73-
}
74-
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
75-
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/master">user2/repo1:master</a>`)
76-
}, 5*time.Second, 100*time.Millisecond)
67+
t.Run("DetectDefaultBranch", func(t *testing.T) {
68+
// the repo shows a prompt to "sync fork" (defaults to the default branch)
69+
require.Eventually(t, func() bool {
70+
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
71+
htmlDoc := NewHTMLParser(t, resp.Body)
72+
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
73+
if mergeUpstreamLink == "" {
74+
return false
75+
}
76+
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
77+
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/master">user2/repo1:master</a>`)
78+
}, 5*time.Second, 100*time.Millisecond)
79+
})
80+
81+
t.Run("DetectSameBranch", func(t *testing.T) {
82+
// if the fork-branch name also exists in the base repo, then use that branch instead
83+
req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/_new/branch/master", map[string]string{
84+
"_csrf": GetUserCSRFToken(t, sessionBaseUser),
85+
"new_branch_name": "fork-branch",
86+
})
87+
sessionBaseUser.MakeRequest(t, req, http.StatusSeeOther)
88+
89+
require.Eventually(t, func() bool {
90+
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
91+
htmlDoc := NewHTMLParser(t, resp.Body)
92+
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
93+
if mergeUpstreamLink == "" {
94+
return false
95+
}
96+
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
97+
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/fork-branch">user2/repo1:fork-branch</a>`)
98+
}, 5*time.Second, 100*time.Millisecond)
99+
})
77100

78101
// click the "sync fork" button
79102
req = NewRequestWithValues(t, "POST", mergeUpstreamLink, map[string]string{"_csrf": GetUserCSRFToken(t, session)})
80103
session.MakeRequest(t, req, http.StatusOK)
81104
checkFileContent("fork-branch", "test-content-1")
105+
106+
// delete the "fork-branch" from the base repo
107+
req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/delete?name=fork-branch", map[string]string{
108+
"_csrf": GetUserCSRFToken(t, sessionBaseUser),
109+
})
110+
sessionBaseUser.MakeRequest(t, req, http.StatusOK)
82111
})
83112

84113
t.Run("BaseChangeAfterHeadChange", func(t *testing.T) {

0 commit comments

Comments
 (0)