10000 Improve sync fork behavior by wxiaoguang · Pull Request #33319 · go-gitea/gitea · GitHub
[go: up one dir, main page]
Skip to content

Improve sync fork behavior #33319

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

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Improve sync fork behavior #33319

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca32157
fix
wxiaoguang Jan 18, 2025
3ec97d0
fix get branch
wxiaoguang Jan 18, 2025
0289193
Merge branch 'main' into fix-sync-fork
wxiaoguang Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions models/git/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ func GetBranch(ctx context.Context, repoID int64, branchName string) (*Branch, e
BranchName: branchName,
}
}
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take this on.

// It causes inconsistency with `GetBranches` and `git.GetBranch`, and will lead to strange bugs
// In the future, there should be 2 functions: `GetBranchExisting` and `GetBranchWithDeleted`
return &branch, nil
}

Expand Down
2 changes: 1 addition & 1 deletion options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1953,7 +1953,7 @@ pulls.upstream_diverging_prompt_behind_1 = This branch is %[1]d commit behind %[
pulls.upstream_diverging_prompt_behind_n = This branch is %[1]d commits behind %[2]s
pulls.upstream_diverging_prompt_base_newer = The base branch %s has new changes
pulls.upstream_diverging_merge = Sync fork
pulls.upstream_diverging_merge_confirm = Would you like to merge base repository's default branch onto this repository's branch %s?
pulls.upstream_diverging_merge_confirm = Would you like to merge "%[1]s" onto "%[2]s"?

pull.deleted_branch = (deleted):%s
pull.agit_documentation = Review documentation about AGit
Expand Down
17 changes: 15 additions & 2 deletions services/repository/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,12 @@ func SetRepoDefaultBranch(ctx context.Context, repo *repo_model.Repository, gitR
}

// 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 {
// whether the base branch contains new commits which are not in the head branch
BaseHasNewCommits bool

// 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.
// there could be a case that BaseHasNewCommits=true while the behind/after are both 0 (unable to calculate).
HeadCommitsBehind int
HeadCommitsAhead int
}
Expand All @@ -681,11 +684,20 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo
if err != nil {
return nil, err
}

if headGitBranch.IsDeleted {
return nil, git_model.ErrBranchNotExist{
BranchName: headBranch,
}
}
baseGitBranch, err := git_model.GetBranch(ctx, baseRepo.ID, baseBranch)
if err != nil {
return nil, err
}
if baseGitBranch.IsDeleted {
return nil, git_model.ErrBranchNotExist{
BranchName: baseBranch,
}
}

info := &BranchDivergingInfo{}
if headGitBranch.CommitID == baseGitBranch.CommitID {
Expand Down Expand Up @@ -720,5 +732,6 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo
}

info.HeadCommitsBehind, info.HeadCommitsAhead = diff.Behind, diff.Ahead
info.BaseHasNewCommits = info.HeadCommitsBehind > 0
return info, nil
}
48 changes: 42 additions & 6 deletions services/repository/merge_upstream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
package repository

import (
"context"
"errors"
"fmt"

issue_model "code.gitea.io/gitea/models/issues"
Expand All @@ -18,16 +18,24 @@ import (
)

// 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) {
func MergeUpstream(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_model.Repository, branch string) (mergeStyle string, err error) {
if err = repo.MustNotBeArchived(); err != nil {
return "", err
}
if err = repo.GetBaseRepo(ctx); err != nil {
return "", err
}
divergingInfo, err := GetUpstreamDivergingInfo(ctx, repo, branch)
if err != nil {
return "", err
}
if !divergingInfo.BaseBranchHasNewCommits {
return "up-to-date", nil
}

err = git.Push(ctx, repo.BaseRepo.RepoPath(), git.PushOptions{
Remote: repo.RepoPath(),
Branch: fmt.Sprintf("%s:%s", repo.BaseRepo.DefaultBranch, branch),
Branch: fmt.Sprintf("%s:%s", divergingInfo.BaseBranchName, branch),
Env: repo_module.PushingEnvironment(doer, repo),
})
if err == nil {
Expand Down Expand Up @@ -59,7 +67,7 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.
BaseRepoID: repo.BaseRepo.ID,
BaseRepo: repo.BaseRepo,
HeadBranch: branch, // maybe HeadCommitID is not needed
BaseBranch: repo.BaseRepo.DefaultBranch,
BaseBranch: divergingInfo.BaseBranchName,
}
fakeIssue.PullRequest = fakePR
err = pull.Update(ctx, fakePR, doer, "merge upstream", false)
Expand All @@ -69,8 +77,15 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.
return "merge", nil
}

// UpstreamDivergingInfo is also used in templates, so it needs to search for all references before changing it.
type UpstreamDivergingInfo struct {
BaseBranchName string
BaseBranchHasNewCommits bool
HeadBranchCommitsBehind int
}

// 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, forkRepo *repo_model.Repository, forkBranch string) (*BranchDivergingInfo, error) {
func GetUpstreamDivergingInfo(ctx reqctx.RequestContext, forkRepo *repo_model.Repository, forkBranch string) (*UpstreamDivergingInfo, error) {
if !forkRepo.IsFork {
return nil, util.NewInvalidArgumentErrorf("repo is not a fork")
}
Expand All @@ -83,5 +98,26 @@ func GetUpstreamDivergingInfo(ctx reqctx.RequestContext, forkRepo *repo_model.Re
return nil, err
}

return GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkRepo.BaseRepo.DefaultBranch, forkRepo, forkBranch)
// Do the best to follow the GitHub's behavior, suppose there is a `branch-a` in fork repo:
// * if `branch-a` exists in base repo: try to sync `base:branch-a` to `fork:branch-a`
// * if `branch-a` doesn't exist in base repo: try to sync `base:main` to `fork:branch-a`
info, err := GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkBranch, forkRepo, forkBranch)
if err == nil {
return &UpstreamDivergingInfo{
BaseBranchName: forkBranch,
BaseBranchHasNewCommits: info.BaseHasNewCommits,
HeadBranchCommitsBehind: info.HeadCommitsBehind,
}, nil
}
if errors.Is(err, util.ErrNotExist) {
info, err = GetBranchDivergingInfo(ctx, forkRepo.BaseRepo, forkRepo.BaseRepo.DefaultBranch, forkRepo, forkBranch)
if err == nil {
return &UpstreamDivergingInfo{
BaseBranchName: forkRepo.BaseRepo.DefaultBranch,
BaseBranchHasNewCommits: info.BaseHasNewCommits,
HeadBranchCommitsBehind: info.HeadCommitsBehind,
}, nil
}
}
return nil, err
}
14 changes: 8 additions & 6 deletions templates/repo/code/upstream_diverging_info.tmpl
View file
Open in desktop
< 9E81 /table>
51 changes: 40 additions & 11 deletions tests/integration/repo_merge_upstream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
{{if and .UpstreamDivergingInfo (or .UpstreamDivergingInfo.BaseHasNewCommits .UpstreamDivergingInfo.HeadCommitsBehind)}}
{{if and .UpstreamDivergingInfo .UpstreamDivergingInfo.BaseBranchHasNewCommits}}
<div class="ui message flex-text-block">
<div class="tw-flex-1">
{{$upstreamLink := printf "%s/src/branch/%s" .Repository.BaseRepo.Link (.Repository.BaseRepo.DefaultBranch|PathEscapeSegments)}}
{{$upstreamHtml := HTMLFormat `<a href="%s">%s:%s</a>` $upstreamLink .Repository.BaseRepo.FullName .Repository.BaseRepo.DefaultBranch}}
{{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}}
{{$upstreamLink := printf "%s/src/branch/%s" .Repository.BaseRepo.Link (.UpstreamDivergingInfo.BaseBranchName|PathEscapeSegments)}}
{{$upstreamRepoBranchDisplay := HTMLFormat "%s:%s" .Repository.BaseRepo.FullName .UpstreamDivergingInfo.BaseBranchName}}
{{$thisRepoBranchDisplay := HTMLFormat "%s:%s" .Repository.FullName .BranchName}}
{{$upstreamHtml := HTMLFormat `<a href="%s">%s</a>` $upstreamLink $upstreamRepoBranchDisplay}}
{{if .UpstreamDivergingInfo.HeadBranchCommitsBehind}}
{{ctx.Locale.TrN .UpstreamDivergingInfo.HeadBranchCommitsBehind "repo.pulls.upstream_diverging_prompt_behind_1" "repo.pulls.upstream_diverging_prompt_behind_n" .UpstreamDivergingInfo.HeadBranchCommitsBehind $upstreamHtml}}
{{else}}
{{ctx.Locale.Tr "repo.pulls.upstream_diverging_prompt_base_newer" $upstreamHtml}}
{{end}}
</div>
{{if .CanWriteCode}}
<button class="ui compact primary button tw-m-0 link-action"
data-modal-confirm-header="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge"}}"
data-modal-confirm-content="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge_confirm" .BranchName}}"
data-modal-confirm-content="{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge_confirm" $upstreamRepoBranchDisplay $thisRepoBranchDisplay}}"
data-url="{{.Repository.Link}}/branches/merge-upstream?branch={{.BranchName}}">
{{ctx.Locale.Tr "repo.pulls.upstream_diverging_merge"}}
</button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,54 @@ func TestRepoMergeUpstream(t *testing.T) {

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

// the repo shows a prompt to "sync fork"
var mergeUpstreamLink string
require.Eventually(t, func() bool {
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
if mergeUpstreamLink == "" {
return false
}
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/master">user2/repo1:master</a>`)
}, 5*time.Second, 100*time.Millisecond)
t.Run("DetectDefaultBranch", func(t *testing.T) {
// the repo shows a prompt to "sync fork" (defaults to the default branch)
require.Eventually(t, func() bool {
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
if mergeUpstreamLink == "" {
return false
}
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/master">user2/repo1:master</a>`)
}, 5*time.Second, 100*time.Millisecond)
})

t.Run("DetectSameBranch", func(t *testing.T) {
// if the fork-branch name also exists in the base repo, then use that branch instead
req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/_new/branch/master", map[string]string{
"_csrf": GetUserCSRFToken(t, sessionBaseUser),
"new_branch_name": "fork-branch",
})
sessionBaseUser.MakeRequest(t, req, http.StatusSeeOther)

require.Eventually(t, func() bool {
resp := session.MakeRequest(t, NewRequestf(t, "GET", "/%s/test-repo-fork/src/branch/fork-branch", forkUser.Name), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
mergeUpstreamLink = queryMergeUpstreamButtonLink(htmlDoc)
if mergeUpstreamLink == "" {
return false
}
respMsg, _ := htmlDoc.Find(".ui.message:not(.positive)").Html()
return strings.Contains(respMsg, `This branch is 1 commit behind <a href="/user2/repo1/src/branch/fork-branch">user2/repo1:fork-branch</a>`)
}, 5*time.Second, 100*time.Millisecond)
})

// click the "sync fork" button
req = NewRequestWithValues(t, "POST", mergeUpstreamLink, map[string]string{"_csrf": GetUserCSRFToken(t, session)})
session.MakeRequest(t, req, http.StatusOK)
checkFileContent("fork-branch", "test-content-1")

// delete the "fork-branch" from the base repo
req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/delete?name=fork-branch", map[string]string{
"_csrf": GetUserCSRFToken(t, sessionBaseUser),
})
sessionBaseUser.MakeRequest(t, req, http.StatusOK)
})

t.Run("BaseChangeAfterHeadChange", func(t *testing.T) {
Expand Down
Loading
0