Improve sync fork behavior (#33319) (#33332)

wxiaoguang · web-flow · commit 8f45a11919ca · 2025-01-20T07:50:38.000Z
Backport #33319 Fix #33271 The only conflict is `reqctx` in `services/repository/merge_upstream.go`, which could keep using `context.Context` in 1.23
diff --git a/models/git/branch.go b/models/git/branch.go
@@ -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.
+	// 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
 }
 
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
@@ -1951,7 +1951,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
diff --git a/services/repository/branch.go b/services/repository/branch.go
@@ -638,9 +638,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
 }
@@ -651,11 +654,20 @@ func GetBranchDivergingInfo(ctx context.Context, baseRepo *repo_model.Repository
 	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 {
@@ -692,5 +704,6 @@ func GetBranchDivergingInfo(ctx context.Context, baseRepo *repo_model.Repository
 	}
 
 	info.HeadCommitsBehind, info.HeadCommitsAhead = diff.Behind, diff.Ahead
+	info.BaseHasNewCommits = info.HeadCommitsBehind > 0
 	return info, nil
 }
diff --git a/services/repository/merge_upstream.go b/services/repository/merge_upstream.go
@@ -5,6 +5,7 @@ package repository
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	issue_model "code.gitea.io/gitea/models/issues"
@@ -24,9 +25,17 @@ func MergeUpstream(ctx context.Context, doer *user_model.User, repo *repo_model.
 	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 {
@@ -58,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)
@@ -68,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 context.Context, forkRepo *repo_model.Repository, forkBranch string) (*BranchDivergingInfo, error) {
+func GetUpstreamDivergingInfo(ctx context.Context, forkRepo *repo_model.Repository, forkBranch string) (*UpstreamDivergingInfo, error) {
 	if !forkRepo.IsFork {
 		return nil, util.NewInvalidArgumentErrorf("repo is not a fork")
 	}
@@ -82,5 +98,26 @@ func GetUpstreamDivergingInfo(ctx context.Context, forkRepo *repo_model.Reposito
 		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
 }
diff --git a/templates/repo/code/upstream_diverging_info.tmpl b/templates/repo/code/upstream_diverging_info.tmpl
@@ -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>
diff --git a/tests/integration/repo_merge_upstream_test.go b/tests/integration/repo_merge_upstream_test.go
@@ -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) {

Original file line number	Diff line number	Diff line change
`@@ -167,6 +167,9 @@ func GetBranch(ctx context.Context, repoID int64, branchName string) (*Branch, e`
`167`	`167`	`BranchName: branchName,`
`168`	`168`	`}`
`169`	`169`	`}`
	`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`
`170`	`173`	`return &branch, nil`
`171`	`174`	`}`
`172`	`175`