From 9b92c18370fffd38e813472927cabb1799f6ac22 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 14:58:23 -0700 Subject: [PATCH 01/12] improve create pull request head ref --- models/issues/pull.go | 1 - routers/web/repo/issue_comment.go | 1 - routers/web/repo/pull_review.go | 7 +- routers/web/repo/pull_review_test.go | 4 +- services/agit/agit.go | 64 ++++++------- services/pull/patch.go | 5 +- services/pull/pull.go | 136 ++++++++++----------------- services/pull/temp_repo.go | 3 - tests/integration/git_misc_test.go | 3 +- 9 files changed, 93 insertions(+), 131 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 7a37b627e1bd0..f9e1551738a02 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -138,7 +138,6 @@ type PullRequest struct { BaseRepoID int64 `xorm:"INDEX"` BaseRepo *repo_model.Repository `xorm:"-"` HeadBranch string - HeadCommitID string `xorm:"-"` BaseBranch string MergeBase string `xorm:"VARCHAR(64)"` AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index cb5b2d801952d..4c9b78d69c95e 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -96,7 +96,6 @@ func NewComment(ctx *context.Context) { // Regenerate patch and test conflict. if pr == nil { - issue.PullRequest.HeadCommitID = "" pull_service.StartPullRequestCheckImmediately(ctx, issue.PullRequest) } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 18e14e9b224c4..ceef7a637bed8 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -318,7 +318,12 @@ func UpdateViewedFiles(ctx *context.Context) { // Expect the review to have been now if no head commit was supplied if data.HeadCommitSHA == "" { - data.HeadCommitSHA = pull.HeadCommitID + data.HeadCommitSHA, err = ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + if err != nil { + log.Warn("Attempted to get ref commit id: %v", err) + ctx.Resp.WriteHeader(http.StatusBadRequest) + return + } } updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files)) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 42223c1d9c616..34821fed559c6 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -41,7 +41,9 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { - comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadCommitID, nil) + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + assert.NoError(t, err) + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, headCommitID, nil) require.NoError(t, err) comment.Invalidated = true diff --git a/services/agit/agit.go b/services/agit/agit.go index 9d2d122cfb3f5..4450f056eb2f2 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -142,21 +142,21 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } pr := &issues_model.PullRequest{ - HeadRepoID: repo.ID, - BaseRepoID: repo.ID, - HeadBranch: headBranch, - HeadCommitID: opts.NewCommitIDs[i], - BaseBranch: baseBranchName, - HeadRepo: repo, - BaseRepo: repo, - MergeBase: "", - Type: issues_model.PullRequestGitea, - Flow: issues_model.PullRequestFlowAGit, + HeadRepoID: repo.ID, + BaseRepoID: repo.ID, + HeadBranch: headBranch, + BaseBranch: baseBranchName, + HeadRepo: repo, + BaseRepo: repo, + MergeBase: "", + Type: issues_model.PullRequestGitea, + Flow: issues_model.PullRequestFlowAGit, } prOpts := &pull_service.NewPullRequestOptions{ - Repo: repo, - Issue: prIssue, - PullRequest: pr, + Repo: repo, + Issue: prIssue, + PullRequest: pr, + HeadCommitID: opts.NewCommitIDs[i], } if err := pull_service.NewPullRequest(ctx, prOpts); err != nil { return nil, err @@ -214,35 +214,29 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } } - // Store old commit ID for review staleness checking - oldHeadCommitID := pr.HeadCommitID - - pr.HeadCommitID = opts.NewCommitIDs[i] - if err = pull_service.UpdateRef(ctx, pr); err != nil { + if err = pull_service.UpdatePullRequestAgitFlowHead(ctx, pr, opts.NewCommitIDs[i]); err != nil { return nil, fmt.Errorf("failed to update pull ref. Error: %w", err) } // Mark existing reviews as stale when PR content changes (same as regular GitHub flow) - if oldHeadCommitID != opts.NewCommitIDs[i] { - if err := issues_model.MarkReviewsAsStale(ctx, pr.IssueID); err != nil { - log.Error("MarkReviewsAsStale: %v", err) - } + if err := issues_model.MarkReviewsAsStale(ctx, pr.IssueID); err != nil { + log.Error("MarkReviewsAsStale: %v", err) + } - // Dismiss all approval reviews if protected branch rule item enabled - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) - if err != nil { - log.Error("GetFirstMatchProtectedBranchRule: %v", err) - } - if pb != nil && pb.DismissStaleApprovals { - if err := pull_service.DismissApprovalReviews(ctx, pusher, pr); err != nil { - log.Error("DismissApprovalReviews: %v", err) - } + // Dismiss all approval reviews if protected branch rule item enabled + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { + log.Error("GetFirstMatchProtectedBranchRule: %v", err) + } + if pb != nil && pb.DismissStaleApprovals { + if err := pull_service.DismissApprovalReviews(ctx, pusher, pr); err != nil { + log.Error("DismissApprovalReviews: %v", err) } + } - // Mark reviews for the new commit as not stale - if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitIDs[i]); err != nil { - log.Error("MarkReviewsAsNotStale: %v", err) - } + // Mark reviews for the new commit as not stale + if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitIDs[i]); err != nil { + log.Error("MarkReviewsAsNotStale: %v", err) } pull_service.StartPullRequestCheckImmediately(ctx, pr) diff --git a/services/pull/patch.go b/services/pull/patch.go index 9d9b8d0d076eb..d91a1f63335ae 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -100,11 +100,12 @@ func testPullRequestTmpRepoBranchMergeable(ctx context.Context, prCtx *prTmpRepo } } pr.MergeBase = strings.TrimSpace(pr.MergeBase) - if pr.HeadCommitID, err = gitRepo.GetRefCommitID(git.BranchPrefix + "tracking"); err != nil { + headCommitID, err := gitRepo.GetRefCommitID(git.BranchPrefix + "tracking") + if err != nil { return fmt.Errorf("GetBranchCommitID: can't find commit ID for head: %w", err) } - if pr.HeadCommitID == pr.MergeBase { + if headCommitID == pr.MergeBase { pr.Status = issues_model.PullRequestStatusAncestor return nil } diff --git a/services/pull/pull.go b/services/pull/pull.go index 20d33b53311c7..c31263c5e1fa9 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -43,6 +43,7 @@ func getPullWorkingLockKey(prID int64) string { type NewPullRequestOptions struct { Repo *repo_model.Repository Issue *issues_model.Issue + HeadCommitID string LabelIDs []int64 AttachmentUUIDs []string PullRequest *issues_model.PullRequest @@ -53,6 +54,10 @@ type NewPullRequestOptions struct { // NewPullRequest creates new pull request with labels for repository. func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { + if opts.PullRequest.Flow == issues_model.PullRequestFlowAGit && opts.HeadCommitID == "" { + return errors.New("head commit ID cannot be empty for agit flow") + } + repo, issue, labelIDs, uuids, pr, assigneeIDs := opts.Repo, opts.Issue, opts.LabelIDs, opts.AttachmentUUIDs, opts.PullRequest, opts.AssigneeIDs if err := issue.LoadPoster(ctx); err != nil { return err @@ -98,13 +103,6 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } - divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) - if err != nil { - return err - } - pr.CommitsAhead = divergence.Ahead - pr.CommitsBehind = divergence.Behind - assigneeCommentMap := make(map[int64]*issues_model.Comment) // add first push codes comment @@ -131,15 +129,27 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { pr.Issue = issue issue.PullRequest = pr - if pr.Flow == issues_model.PullRequestFlowGithub { - err = PushToBaseRepo(ctx, pr) + // update head commit id into git repository + if pr.Flow == issues_model.PullRequestFlowAGit { + err = UpdatePullRequestAgitFlowHead(ctx, pr, opts.HeadCommitID) } else { - err = UpdateRef(ctx, pr) + err = UpdatePullRequestGithubFlowHead(ctx, pr) } if err != nil { return err } + // update commits ahead and behind + divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + if err != nil { + return err + } + pr.CommitsAhead = divergence.Ahead + pr.CommitsBehind = divergence.Behind + if _, err := db.GetEngine(ctx).ID(pr.ID).Cols("commits_ahead", "commits_behind").NoAutoTime().Update(pr); err != nil { + return err + } + if _, err := CreatePushPullComment(ctx, issue.Poster, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName(), false); err != nil { return err } @@ -388,7 +398,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { for _, pr := range prs { log.Trace("Updating PR[%d]: composing new test task", pr.ID) if pr.Flow == issues_model.PullRequestFlowGithub { - if err := PushToBaseRepo(ctx, pr); err != nil { + if err := UpdatePullRequestGithubFlowHead(ctx, pr); err != nil { log.Error("PushToBaseRepo: %v", err) continue } @@ -546,68 +556,6 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, return false, nil } -// PushToBaseRepo pushes commits from branches of head repository to -// corresponding branches of base repository. -// FIXME: Only push branches that are actually updates? -func PushToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) (err error) { - return pushToBaseRepoHelper(ctx, pr, "") -} - -func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, prefixHeadBranch string) (err error) { - log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitHeadRefName()) - - if err := pr.LoadHeadRepo(ctx); err != nil { - log.Error("Unable to load head repository for PR[%d] Error: %v", pr.ID, err) - return err - } - headRepoPath := pr.HeadRepo.RepoPath() - - if err := pr.LoadBaseRepo(ctx); err != nil { - log.Error("Unable to load base repository for PR[%d] Error: %v", pr.ID, err) - return err - } - baseRepoPath := pr.BaseRepo.RepoPath() - - if err = pr.LoadIssue(ctx); err != nil { - return fmt.Errorf("unable to load issue %d for pr %d: %w", pr.IssueID, pr.ID, err) - } - if err = pr.Issue.LoadPoster(ctx); err != nil { - return fmt.Errorf("unable to load poster %d for pr %d: %w", pr.Issue.PosterID, pr.ID, err) - } - - gitRefName := pr.GetGitHeadRefName() - - if err := git.Push(ctx, headRepoPath, git.PushOptions{ - Remote: baseRepoPath, - Branch: prefixHeadBranch + pr.HeadBranch + ":" + gitRefName, - Force: true, - // Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/... - Env: repo_module.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo), - }); err != nil { - if git.IsErrPushOutOfDate(err) { - // This should not happen as we're using force! - log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err) - return err - } else if git.IsErrPushRejected(err) { - rejectErr := err.(*git.ErrPushRejected) - log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err) - return err - } else if git.IsErrMoreThanOne(err) { - if prefixHeadBranch != "" { - log.Info("Can't push with %s%s", prefixHeadBranch, pr.HeadBranch) - return err - } - log.Info("Retrying to push with %s%s", git.BranchPrefix, pr.HeadBranch) - err = pushToBaseRepoHelper(ctx, pr, git.BranchPrefix) - return err - } - log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err) - return fmt.Errorf("Push: %s:%s %s:%s %w", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), gitRefName, err) - } - - return nil -} - // UpdatePullsRefs update all the PRs head file pointers like /refs/pull/1/head so that it will be dependent by other operations func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *repo_module.PushUpdateOptions) { branch := update.RefFullName.BranchName() @@ -619,27 +567,43 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r for _, pr := range prs { log.Trace("Updating PR[%d]: composing new test task", pr.ID) if pr.Flow == issues_model.PullRequestFlowGithub { - if err := PushToBaseRepo(ctx, pr); err != nil { - log.Error("PushToBaseRepo: %v", err) + if err := UpdatePullRequestGithubFlowHead(ctx, pr); err != nil { + log.Error("UpdatePullRequestHead: %v", err) } } } } } -// UpdateRef update refs/pull/id/head directly for agit flow pull request -func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { - log.Trace("UpdateRef[%d]: upgate pull request ref in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) +func UpdatePullRequestAgitFlowHead(ctx context.Context, pr *issues_model.PullRequest, commitID string) error { + log.Trace("UpdateAgitPullRequestHead[%d]: update pull request head in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) + + _, _, err := git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), commitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + return err +} + +// UpdatePullRequestHeadRef updates the head reference of a pull request +func UpdatePullRequestGithubFlowHead(ctx context.Context, pr *issues_model.PullRequest) error { + log.Trace("UpdatePullRequestHeadRef[%d]: update pull request ref in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) + if err := pr.LoadBaseRepo(ctx); err != nil { - log.Error("Unable to load base repository for PR[%d] Error: %v", pr.ID, err) return err } - _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) - if err != nil { - log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) + if pr.IsSameRepo() { // for agit flow or github flow in the same repository + _, _, err := git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.HeadBranch).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + return err + } + + // for cross repository pull request + if err := pr.LoadHeadRepo(ctx); err != nil { + return err } + _, _, err := git.NewCommand("fetch", "--no-tags", "--refmap="). + AddDynamicArguments(pr.HeadRepo.RepoPath()). + AddDynamicArguments(fmt.Sprintf("refs/heads/%s:%s", pr.HeadBranch, pr.GetGitHeadRefName())). + RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) return err } @@ -803,12 +767,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ if pr.Flow == issues_model.PullRequestFlowGithub { headCommit, err = gitRepo.GetBranchCommit(pr.HeadBranch) } else { - pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) return "" } - headCommit, err = gitRepo.GetCommit(pr.HeadCommitID) + headCommit, err = gitRepo.GetCommit(headCommitID) } if err != nil { log.Error("Unable to get head commit: %s Error: %v", pr.HeadBranch, err) @@ -1030,11 +994,11 @@ func IsHeadEqualWithBranch(ctx context.Context, pr *issues_model.PullRequest, br return false, err } } else { - pr.HeadCommitID, err = baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + headCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { return false, err } - if headCommit, err = baseGitRepo.GetCommit(pr.HeadCommitID); err != nil { + if headCommit, err = baseGitRepo.GetCommit(headCommitID); err != nil { return false, err } } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 89f150fd92e9d..5d46c9be70388 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -166,13 +166,10 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } trackingBranch := "tracking" - objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) // Fetch head branch var headBranch string if pr.Flow == issues_model.PullRequestFlowGithub { headBranch = git.BranchPrefix + pr.HeadBranch - } else if len(pr.HeadCommitID) == objectFormat.FullLength() { // for not created pull request - headBranch = pr.HeadCommitID } else { headBranch = pr.GetGitHeadRefName() } diff --git a/tests/integration/git_misc_test.go b/tests/integration/git_misc_test.go index eb039c7c1c9cf..aaeb8c3cc6d47 100644 --- a/tests/integration/git_misc_test.go +++ b/tests/integration/git_misc_test.go @@ -170,7 +170,8 @@ func TestAgitReviewStaleness(t *testing.T) { assert.NoError(t, pr.LoadIssue(t.Context())) // Get initial commit ID for the review - initialCommitID := pr.HeadCommitID + initialCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + assert.NoError(t, err) t.Logf("Initial commit ID: %s", initialCommitID) // Create a review on the PR (as user1 reviewing user2's PR) From b1550217535378486c6ce969a84e108c9081a1d3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 15:29:44 -0700 Subject: [PATCH 02/12] Update all get diverging commits --- services/pull/pull.go | 31 +++++++++++++++++-------------- services/pull/update.go | 7 ++++++- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c31263c5e1fa9..49652bd7efaa8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -303,20 +303,17 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer pr.Status = issues_model.PullRequestStatusMergeable } - // Update Commit Divergence - divergence, err := GetDiverging(ctx, pr) - if err != nil { + if err := pr.LoadBaseRepo(ctx); err != nil { return err } - pr.CommitsAhead = divergence.Ahead - pr.CommitsBehind = divergence.Behind - // add first push codes comment - baseGitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) + // update commits ahead and behind + divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { return err } - defer baseGitRepo.Close() + pr.CommitsAhead = divergence.Ahead + pr.CommitsBehind = divergence.Behind return db.WithTx(ctx, func(ctx context.Context) error { if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch", "commits_ahead", "commits_behind"); err != nil { @@ -448,9 +445,10 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) } - divergence, err := GetDiverging(ctx, pr) + + divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { - log.Error("GetDiverging: %v", err) + log.Error("GetDivergingCommits: %v", err) } else { err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) if err != nil { @@ -480,8 +478,13 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } + baseRepo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) + if err != nil { + log.Error("GetRepositoryByID: %v", err) + return + } for _, pr := range prs { - divergence, err := GetDiverging(ctx, pr) + divergence, err := git.GetDivergingCommits(ctx, baseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) @@ -767,9 +770,9 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ if pr.Flow == issues_model.PullRequestFlowGithub { headCommit, err = gitRepo.GetBranchCommit(pr.HeadBranch) } else { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err != nil { - log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) + headCommitID, err1 := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err1 != nil { + log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err1) return "" } headCommit, err = gitRepo.GetCommit(headCommitID) diff --git a/services/pull/update.go b/services/pull/update.go index b8f84e3d658ae..b8882bf924896 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -34,7 +34,12 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } defer releaser() - diffCount, err := GetDiverging(ctx, pr) + if err := pr.LoadBaseRepo(ctx); err != nil { + log.Error("unable to load BaseRepo for PR %-v during update-by-merge: %v", pr, err) + return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) + } + + diffCount, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { return err } else if diffCount.Behind == 0 { From ca0ecf193d3e46ade00618c4eb920ff62536fbb1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 19:33:18 -0700 Subject: [PATCH 03/12] Fix bug --- services/pull/comment.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/pull/comment.go b/services/pull/comment.go index f12edaf0326b0..23b4725b21139 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -5,6 +5,7 @@ package pull import ( "context" + "fmt" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" @@ -52,6 +53,10 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss return nil, nil } + if err := pr.LoadIssue(ctx); err != nil { + return nil, fmt.Errorf("unable to load issue for PR[%d]: %w", pr.ID, err) + } + opts := &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypePullRequestPush, Doer: pusher, From ba25ea6c07e3a89dd552d18d62d95a89d8948e30 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 20:58:29 -0700 Subject: [PATCH 04/12] some improvements --- models/migrations/v1_12/v136.go | 27 +++++------ modules/git/repo.go | 30 ------------- modules/git/repo_test.go | 24 ---------- modules/gitrepo/diverging.go | 45 +++++++++++++++++++ services/pull/pull.go | 11 ++--- services/pull/update.go | 18 +++----- services/repository/branch.go | 18 ++++---- services/repository/files/commit.go | 9 ++-- .../integration/change_default_branch_test.go | 8 ++-- 9 files changed, 86 insertions(+), 104 deletions(-) create mode 100644 modules/gitrepo/diverging.go diff --git a/models/migrations/v1_12/v136.go b/models/migrations/v1_12/v136.go index 0f53278b4623f..aa369526b98d3 100644 --- a/models/migrations/v1_12/v136.go +++ b/models/migrations/v1_12/v136.go @@ -6,11 +6,10 @@ package v1_12 import ( "fmt" "math" - "path/filepath" "strings" "time" - "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -18,15 +17,19 @@ import ( "xorm.io/xorm" ) -func AddCommitDivergenceToPulls(x *xorm.Engine) error { - type Repository struct { - ID int64 `xorm:"pk autoincr"` - OwnerID int64 `xorm:"UNIQUE(s) index"` - OwnerName string - LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"` - Name string `xorm:"INDEX NOT NULL"` - } +type Repository struct { + ID int64 `xorm:"pk autoincr"` + OwnerID int64 `xorm:"UNIQUE(s) index"` + OwnerName string + LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"` + Name string `xorm:"INDEX NOT NULL"` +} +func (r *Repository) RelativePath() string { + return fmt.Sprintf("%s/%s.git", strings.ToLower(r.OwnerName), strings.ToLower(r.Name)) +} + +func AddCommitDivergenceToPulls(x *xorm.Engine) error { type PullRequest struct { ID int64 `xorm:"pk autoincr"` IssueID int64 `xorm:"INDEX"` @@ -85,12 +88,10 @@ func AddCommitDivergenceToPulls(x *xorm.Engine) error { log.Error("Missing base repo with id %d for PR ID %d", pr.BaseRepoID, pr.ID) continue } - userPath := filepath.Join(setting.RepoRootPath, strings.ToLower(baseRepo.OwnerName)) - repoPath := filepath.Join(userPath, strings.ToLower(baseRepo.Name)+".git") gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index) - divergence, err := git.GetDivergingCommits(graceful.GetManager().HammerContext(), repoPath, pr.BaseBranch, gitRefName) + divergence, err := gitrepo.GetDivergingCommits(graceful.GetManager().HammerContext(), baseRepo, pr.BaseBranch, gitRefName) if err != nil { log.Warn("Could not recalculate Divergence for pull: %d", pr.ID) pr.CommitsAhead = 0 diff --git a/modules/git/repo.go b/modules/git/repo.go index 8e7b427bd0ed9..9efc37f465516 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -242,36 +242,6 @@ func GetLatestCommitTime(ctx context.Context, repoPath string) (time.Time, error return time.Parse("Mon Jan _2 15:04:05 2006 -0700", commitTime) } -// DivergeObject represents commit count diverging commits -type DivergeObject struct { - Ahead int - Behind int -} - -// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch -func GetDivergingCommits(ctx context.Context, repoPath, baseBranch, targetBranch string) (do DivergeObject, err error) { - cmd := NewCommand("rev-list", "--count", "--left-right"). - AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") - stdout, _, err := cmd.RunStdString(ctx, &RunOpts{Dir: repoPath}) - if err != nil { - return do, err - } - left, right, found := strings.Cut(strings.Trim(stdout, "\n"), "\t") - if !found { - return do, fmt.Errorf("git rev-list output is missing a tab: %q", stdout) - } - - do.Behind, err = strconv.Atoi(left) - if err != nil { - return do, err - } - do.Ahead, err = strconv.Atoi(right) - if err != nil { - return do, err - } - return do, nil -} - // CreateBundle create bundle content to the target path func (repo *Repository) CreateBundle(ctx context.Context, commit string, out io.Writer) error { tmp, cleanup, err := setting.AppDataTempDir("git-repo-content").MkdirTempRandom("gitea-bundle") diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index 813844d1ea131..26ee3a091a269 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -29,27 +29,3 @@ func TestRepoIsEmpty(t *testing.T) { assert.NoError(t, err) assert.True(t, isEmpty) } - -func TestRepoGetDivergingCommits(t *testing.T) { - bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") - do, err := GetDivergingCommits(t.Context(), bareRepo1Path, "master", "branch2") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 1, - Behind: 5, - }, do) - - do, err = GetDivergingCommits(t.Context(), bareRepo1Path, "master", "master") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 0, - Behind: 0, - }, do) - - do, err = GetDivergingCommits(t.Context(), bareRepo1Path, "master", "test") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 0, - Behind: 2, - }, do) -} diff --git a/modules/gitrepo/diverging.go b/modules/gitrepo/diverging.go new file mode 100644 index 0000000000000..ef792426dbc97 --- /dev/null +++ b/modules/gitrepo/diverging.go @@ -0,0 +1,45 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "context" + "fmt" + "strconv" + "strings" + + "code.gitea.io/gitea/modules/git" +) + +// DivergeObject represents commit count diverging commits +type DivergeObject struct { + Ahead int + Behind int +} + +// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch +func GetDivergingCommits(ctx context.Context, repo Repository, baseBranch, targetBranch string) (*DivergeObject, error) { + cmd := git.NewCommand("rev-list", "--count", "--left-right"). + AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") + stdout, _, err1 := cmd.RunStdString(ctx, &git.RunOpts{Dir: repoPath(repo)}) + if err1 != nil { + return nil, err1 + } + left, right, found := strings.Cut(strings.Trim(stdout, "\n"), "\t") + if !found { + return nil, fmt.Errorf("git rev-list output is missing a tab: %q", stdout) + } + + var do DivergeObject + var err error + do.Behind, err = strconv.Atoi(left) + if err != nil { + return nil, err + } + do.Ahead, err = strconv.Atoi(right) + if err != nil { + return nil, err + } + return &do, nil +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 49652bd7efaa8..518b98e8ae585 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -140,7 +140,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } // update commits ahead and behind - divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + divergence, err := GetDiverging(ctx, pr) if err != nil { return err } @@ -308,7 +308,7 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer } // update commits ahead and behind - divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + divergence, err := GetDiverging(ctx, pr) if err != nil { return err } @@ -446,9 +446,9 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Error("MarkReviewsAsNotStale: %v", err) } - divergence, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + divergence, err := GetDiverging(ctx, pr) if err != nil { - log.Error("GetDivergingCommits: %v", err) + log.Error("GetDiverging: %v", err) } else { err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) if err != nil { @@ -484,7 +484,8 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { return } for _, pr := range prs { - divergence, err := git.GetDivergingCommits(ctx, baseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + pr.BaseRepo = baseRepo + divergence, err := GetDiverging(ctx, pr) if err != nil { if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) diff --git a/services/pull/update.go b/services/pull/update.go index b8882bf924896..967565bef8e97 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -14,7 +14,7 @@ import ( 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/gitrepo" "code.gitea.io/gitea/modules/globallock" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/repository" @@ -39,7 +39,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } - diffCount, err := git.GetDivergingCommits(ctx, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) + diffCount, err := GetDiverging(ctx, pr) if err != nil { return err } else if diffCount.Behind == 0 { @@ -178,17 +178,9 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, } // GetDiverging determines how many commits a PR is ahead or behind the PR base branch -func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*git.DivergeObject, error) { - log.Trace("GetDiverging[%-v]: compare commits", pr) - prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) - if err != nil { - if !git_model.IsErrBranchNotExist(err) { - log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) - } +func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*gitrepo.DivergeObject, error) { + if err := pr.LoadBaseRepo(ctx); err != nil { return nil, err } - defer cancel() - - diff, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) - return &diff, err + return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) } diff --git a/services/repository/branch.go b/services/repository/branch.go index df7202227aa70..9d69a521ded95 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -123,23 +123,23 @@ func getDivergenceCacheKey(repoID int64, branchName string) string { } // getDivergenceFromCache gets the divergence from cache -func getDivergenceFromCache(repoID int64, branchName string) (*git.DivergeObject, bool) { +func getDivergenceFromCache(repoID int64, branchName string) (*gitrepo.DivergeObject, bool) { data, ok := cache.GetCache().Get(getDivergenceCacheKey(repoID, branchName)) - res := git.DivergeObject{ + res := &gitrepo.DivergeObject{ Ahead: -1, Behind: -1, } if !ok || data == "" { - return &res, false + return res, false } if err := json.Unmarshal(util.UnsafeStringToBytes(data), &res); err != nil { log.Error("json.UnMarshal failed: %v", err) - return &res, false + return res, false } - return &res, true + return res, true } -func putDivergenceFromCache(repoID int64, branchName string, divergence *git.DivergeObject) error { +func putDivergenceFromCache(repoID int64, branchName string, divergence *gitrepo.DivergeObject) error { bs, err := json.Marshal(divergence) if err != nil { return err @@ -178,7 +178,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g p := protectedBranches.GetFirstMatched(branchName) isProtected := p != nil - var divergence *git.DivergeObject + var divergence *gitrepo.DivergeObject // it's not default branch if repo.DefaultBranch != dbBranch.Name && !dbBranch.IsDeleted { @@ -199,7 +199,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g if divergence == nil { // tolerate the error that we cannot get divergence - divergence = &git.DivergeObject{Ahead: -1, Behind: -1} + divergence = &gitrepo.DivergeObject{Ahead: -1, Behind: -1} } pr, err := issues_model.GetLatestPullRequestByHeadInfo(ctx, repo.ID, branchName) @@ -720,7 +720,7 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo // 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) + diff, err := gitrepo.GetDivergingCommits(ctx, baseRepo, baseGitBranch.CommitID, headGitBranch.CommitID) if err != nil { info.BaseHasNewCommits = baseGitBranch.UpdatedUnix > headGitBranch.UpdatedUnix if headRepo.IsFork && info.BaseHasNewCommits { diff --git a/services/repository/files/commit.go b/services/repository/files/commit.go index 3cc326d065b16..e71b1325c6985 100644 --- a/services/repository/files/commit.go +++ b/services/repository/files/commit.go @@ -8,17 +8,14 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/structs" asymkey_service "code.gitea.io/gitea/services/asymkey" ) // CountDivergingCommits determines how many commits a branch is ahead or behind the repository's base branch -func CountDivergingCommits(ctx context.Context, repo *repo_model.Repository, branch string) (*git.DivergeObject, error) { - divergence, err := git.GetDivergingCommits(ctx, repo.RepoPath(), repo.DefaultBranch, branch) - if err != nil { - return nil, err - } - return &divergence, nil +func CountDivergingCommits(ctx context.Context, repo *repo_model.Repository, branch string) (*gitrepo.DivergeObject, error) { + return gitrepo.GetDivergingCommits(ctx, repo, repo.DefaultBranch, branch) } // GetPayloadCommitVerification returns the verification information of a commit diff --git a/tests/integration/change_default_branch_test.go b/tests/integration/change_default_branch_test.go index 9b61cff9fd082..6b752f6d3e0c2 100644 --- a/tests/integration/change_default_branch_test.go +++ b/tests/integration/change_default_branch_test.go @@ -12,7 +12,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -43,7 +43,7 @@ func TestChangeDefaultBranch(t *testing.T) { session.MakeRequest(t, req, http.StatusNotFound) } -func checkDivergence(t *testing.T, session *TestSession, branchesURL, expectedDefaultBranch string, expectedBranchToDivergence map[string]git.DivergeObject) { +func checkDivergence(t *testing.T, session *TestSession, branchesURL, expectedDefaultBranch string, expectedBranchToDivergence map[string]*gitrepo.DivergeObject) { req := NewRequest(t, "GET", branchesURL) resp := session.MakeRequest(t, req, http.StatusOK) @@ -92,7 +92,7 @@ func TestChangeDefaultBranchDivergence(t *testing.T) { settingsBranchesURL := fmt.Sprintf("/%s/%s/settings/branches", owner.Name, repo.Name) // check branch divergence before switching default branch - expectedBranchToDivergenceBefore := map[string]git.DivergeObject{ + expectedBranchToDivergenceBefore := map[string]*gitrepo.DivergeObject{ "not-signed": { Ahead: 0, Behind: 0, @@ -119,7 +119,7 @@ func TestChangeDefaultBranchDivergence(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) // check branch divergence after switching default branch - expectedBranchToDivergenceAfter := map[string]git.DivergeObject{ + expectedBranchToDivergenceAfter := map[string]*gitrepo.DivergeObject{ "master": { Ahead: 1, Behind: 0, From cbce199b98fb06aac25e88277f671500ee3f19ef Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 21:05:21 -0700 Subject: [PATCH 05/12] some improvements --- services/pull/update.go | 19 +++++++------------ services/repository/branch.go | 13 ++++++------- services/repository/files/commit.go | 7 ------- 3 files changed, 13 insertions(+), 26 deletions(-) diff --git a/services/pull/update.go b/services/pull/update.go index 967565bef8e97..6fa5a127fe504 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -34,18 +34,6 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } defer releaser() - if err := pr.LoadBaseRepo(ctx); err != nil { - log.Error("unable to load BaseRepo for PR %-v during update-by-merge: %v", pr, err) - return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) - } - - diffCount, err := GetDiverging(ctx, pr) - if err != nil { - return err - } else if diffCount.Behind == 0 { - return fmt.Errorf("HeadBranch of PR %d is up to date", pr.Index) - } - if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("unable to load BaseRepo for %-v during update-by-merge: %v", pr, err) return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) @@ -63,6 +51,13 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load HeadRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } + diffCount, err := GetDiverging(ctx, pr) + if err != nil { + return err + } else if diffCount.Behind == 0 { + return fmt.Errorf("HeadBranch of PR %d is up to date", pr.Index) + } + defer func() { go AddTestPullRequestTask(TestPullRequestOptions{ RepoID: pr.BaseRepo.ID, diff --git a/services/repository/branch.go b/services/repository/branch.go index 9d69a521ded95..1a34121d9906b 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -33,7 +33,6 @@ import ( actions_service "code.gitea.io/gitea/services/actions" notify_service "code.gitea.io/gitea/services/notify" release_service "code.gitea.io/gitea/services/release" - files_service "code.gitea.io/gitea/services/repository/files" "xorm.io/builder" ) @@ -125,18 +124,18 @@ func getDivergenceCacheKey(repoID int64, branchName string) string { // getDivergenceFromCache gets the divergence from cache func getDivergenceFromCache(repoID int64, branchName string) (*gitrepo.DivergeObject, bool) { data, ok := cache.GetCache().Get(getDivergenceCacheKey(repoID, branchName)) - res := &gitrepo.DivergeObject{ + res := gitrepo.DivergeObject{ Ahead: -1, Behind: -1, } if !ok || data == "" { - return res, false + return &res, false } if err := json.Unmarshal(util.UnsafeStringToBytes(data), &res); err != nil { log.Error("json.UnMarshal failed: %v", err) - return res, false + return &res, false } - return res, true + return &res, true } func putDivergenceFromCache(repoID int64, branchName string, divergence *gitrepo.DivergeObject) error { @@ -186,9 +185,9 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g divergence, cached = getDivergenceFromCache(repo.ID, dbBranch.Name) if !cached { var err error - divergence, err = files_service.CountDivergingCommits(ctx, repo, git.BranchPrefix+branchName) + divergence, err = gitrepo.GetDivergingCommits(ctx, repo, repo.DefaultBranch, git.BranchPrefix+branchName) if err != nil { - log.Error("CountDivergingCommits: %v", err) + log.Error("GetDivergingCommits: %v", err) } else { if err = putDivergenceFromCache(repo.ID, dbBranch.Name, divergence); err != nil { log.Error("putDivergenceFromCache: %v", err) diff --git a/services/repository/files/commit.go b/services/repository/files/commit.go index e71b1325c6985..2e27921494981 100644 --- a/services/repository/files/commit.go +++ b/services/repository/files/commit.go @@ -6,18 +6,11 @@ package files import ( "context" - repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/structs" asymkey_service "code.gitea.io/gitea/services/asymkey" ) -// CountDivergingCommits determines how many commits a branch is ahead or behind the repository's base branch -func CountDivergingCommits(ctx context.Context, repo *repo_model.Repository, branch string) (*gitrepo.DivergeObject, error) { - return gitrepo.GetDivergingCommits(ctx, repo, repo.DefaultBranch, branch) -} - // GetPayloadCommitVerification returns the verification information of a commit func GetPayloadCommitVerification(ctx context.Context, commit *git.Commit) *structs.PayloadCommitVerification { verification := &structs.PayloadCommitVerification{} From 98175f1a939a01a89c4cc4da01fa706cbf53935f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 22:13:51 -0700 Subject: [PATCH 06/12] Revert headcommitid change --- models/issues/pull.go | 1 + routers/web/repo/issue_comment.go | 1 + routers/web/repo/pull_review.go | 7 +------ routers/web/repo/pull_review_test.go | 4 +--- services/agit/agit.go | 29 ++++++++++++++-------------- services/pull/patch.go | 5 ++--- services/pull/pull.go | 17 ++++++++-------- services/pull/temp_repo.go | 3 +++ tests/integration/git_misc_test.go | 2 +- 9 files changed, 33 insertions(+), 36 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index f9e1551738a02..7a37b627e1bd0 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -138,6 +138,7 @@ type PullRequest struct { BaseRepoID int64 `xorm:"INDEX"` BaseRepo *repo_model.Repository `xorm:"-"` HeadBranch string + HeadCommitID string `xorm:"-"` BaseBranch string MergeBase string `xorm:"VARCHAR(64)"` AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 4c9b78d69c95e..cb5b2d801952d 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -96,6 +96,7 @@ func NewComment(ctx *context.Context) { // Regenerate patch and test conflict. if pr == nil { + issue.PullRequest.HeadCommitID = "" pull_service.StartPullRequestCheckImmediately(ctx, issue.PullRequest) } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index ceef7a637bed8..18e14e9b224c4 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -318,12 +318,7 @@ func UpdateViewedFiles(ctx *context.Context) { // Expect the review to have been now if no head commit was supplied if data.HeadCommitSHA == "" { - data.HeadCommitSHA, err = ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitHeadRefName()) - if err != nil { - log.Warn("Attempted to get ref commit id: %v", err) - ctx.Resp.WriteHeader(http.StatusBadRequest) - return - } + data.HeadCommitSHA = pull.HeadCommitID } updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files)) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 34821fed559c6..42223c1d9c616 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -41,9 +41,7 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { - headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitHeadRefName()) - assert.NoError(t, err) - comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, headCommitID, nil) + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadCommitID, nil) require.NoError(t, err) comment.Invalidated = true diff --git a/services/agit/agit.go b/services/agit/agit.go index 4450f056eb2f2..437d9909c97f8 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -142,21 +142,21 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } pr := &issues_model.PullRequest{ - HeadRepoID: repo.ID, - BaseRepoID: repo.ID, - HeadBranch: headBranch, - BaseBranch: baseBranchName, - HeadRepo: repo, - BaseRepo: repo, - MergeBase: "", - Type: issues_model.PullRequestGitea, - Flow: issues_model.PullRequestFlowAGit, + HeadRepoID: repo.ID, + BaseRepoID: repo.ID, + HeadBranch: headBranch, + HeadCommitID: opts.NewCommitIDs[i], + BaseBranch: baseBranchName, + HeadRepo: repo, + BaseRepo: repo, + MergeBase: "", + Type: issues_model.PullRequestGitea, + Flow: issues_model.PullRequestFlowAGit, } prOpts := &pull_service.NewPullRequestOptions{ - Repo: repo, - Issue: prIssue, - PullRequest: pr, - HeadCommitID: opts.NewCommitIDs[i], + Repo: repo, + Issue: prIssue, + PullRequest: pr, } if err := pull_service.NewPullRequest(ctx, prOpts); err != nil { return nil, err @@ -214,7 +214,8 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } } - if err = pull_service.UpdatePullRequestAgitFlowHead(ctx, pr, opts.NewCommitIDs[i]); err != nil { + pr.HeadCommitID = opts.NewCommitIDs[i] + if err = pull_service.UpdatePullRequestAgitFlowHead(ctx, pr, pr.HeadCommitID); err != nil { return nil, fmt.Errorf("failed to update pull ref. Error: %w", err) } diff --git a/services/pull/patch.go b/services/pull/patch.go index d91a1f63335ae..9d9b8d0d076eb 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -100,12 +100,11 @@ func testPullRequestTmpRepoBranchMergeable(ctx context.Context, prCtx *prTmpRepo } } pr.MergeBase = strings.TrimSpace(pr.MergeBase) - headCommitID, err := gitRepo.GetRefCommitID(git.BranchPrefix + "tracking") - if err != nil { + if pr.HeadCommitID, err = gitRepo.GetRefCommitID(git.BranchPrefix + "tracking"); err != nil { return fmt.Errorf("GetBranchCommitID: can't find commit ID for head: %w", err) } - if headCommitID == pr.MergeBase { + if pr.HeadCommitID == pr.MergeBase { pr.Status = issues_model.PullRequestStatusAncestor return nil } diff --git a/services/pull/pull.go b/services/pull/pull.go index 518b98e8ae585..2bb0582c293b5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -43,7 +43,6 @@ func getPullWorkingLockKey(prID int64) string { type NewPullRequestOptions struct { Repo *repo_model.Repository Issue *issues_model.Issue - HeadCommitID string LabelIDs []int64 AttachmentUUIDs []string PullRequest *issues_model.PullRequest @@ -54,7 +53,7 @@ type NewPullRequestOptions struct { // NewPullRequest creates new pull request with labels for repository. func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { - if opts.PullRequest.Flow == issues_model.PullRequestFlowAGit && opts.HeadCommitID == "" { + if opts.PullRequest.Flow == issues_model.PullRequestFlowAGit && opts.PullRequest.HeadCommitID == "" { return errors.New("head commit ID cannot be empty for agit flow") } @@ -131,7 +130,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { // update head commit id into git repository if pr.Flow == issues_model.PullRequestFlowAGit { - err = UpdatePullRequestAgitFlowHead(ctx, pr, opts.HeadCommitID) + err = UpdatePullRequestAgitFlowHead(ctx, pr, pr.HeadCommitID) } else { err = UpdatePullRequestGithubFlowHead(ctx, pr) } @@ -771,12 +770,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ if pr.Flow == issues_model.PullRequestFlowGithub { headCommit, err = gitRepo.GetBranchCommit(pr.HeadBranch) } else { - headCommitID, err1 := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err1 != nil { - log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err1) + pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err != nil { + log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) return "" } - headCommit, err = gitRepo.GetCommit(headCommitID) + headCommit, err = gitRepo.GetCommit(pr.HeadCommitID) } if err != nil { log.Error("Unable to get head commit: %s Error: %v", pr.HeadBranch, err) @@ -998,11 +997,11 @@ func IsHeadEqualWithBranch(ctx context.Context, pr *issues_model.PullRequest, br return false, err } } else { - headCommitID, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + pr.HeadCommitID, err = baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { return false, err } - if headCommit, err = baseGitRepo.GetCommit(headCommitID); err != nil { + if headCommit, err = baseGitRepo.GetCommit(pr.HeadCommitID); err != nil { return false, err } } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 5d46c9be70388..89f150fd92e9d 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -166,10 +166,13 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } trackingBranch := "tracking" + objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) // Fetch head branch var headBranch string if pr.Flow == issues_model.PullRequestFlowGithub { headBranch = git.BranchPrefix + pr.HeadBranch + } else if len(pr.HeadCommitID) == objectFormat.FullLength() { // for not created pull request + headBranch = pr.HeadCommitID } else { headBranch = pr.GetGitHeadRefName() } diff --git a/tests/integration/git_misc_test.go b/tests/integration/git_misc_test.go index aaeb8c3cc6d47..3d3e03645c095 100644 --- a/tests/integration/git_misc_test.go +++ b/tests/integration/git_misc_test.go @@ -170,7 +170,7 @@ func TestAgitReviewStaleness(t *testing.T) { assert.NoError(t, pr.LoadIssue(t.Context())) // Get initial commit ID for the review - initialCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + initialCommitID := pr.HeadCommitID assert.NoError(t, err) t.Logf("Initial commit ID: %s", initialCommitID) From 2a832b3fff99fadf6f9d7d759331f09441d99d3b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 22:29:33 -0700 Subject: [PATCH 07/12] some improvements --- modules/git/repo_commit_gogit.go | 4 ++-- modules/git/repo_commit_nogogit.go | 4 ++-- modules/git/repo_compare_test.go | 2 +- modules/gitrepo/reference.go | 17 +++++++++++++++++ routers/api/v1/repo/issue.go | 2 +- routers/web/repo/issue_list.go | 2 +- routers/web/repo/issue_new.go | 2 +- services/issue/issue.go | 7 ++++--- services/pull/pull.go | 10 +--------- 9 files changed, 30 insertions(+), 20 deletions(-) create mode 100644 modules/gitrepo/reference.go diff --git a/modules/git/repo_commit_gogit.go b/modules/git/repo_commit_gogit.go index a88902e209dc6..0a07377abc420 100644 --- a/modules/git/repo_commit_gogit.go +++ b/modules/git/repo_commit_gogit.go @@ -41,8 +41,8 @@ func (repo *Repository) SetReference(name, commitID string) error { return repo.gogitRepo.Storer.SetReference(plumbing.NewReferenceFromStrings(name, commitID)) } -// RemoveReference removes the given reference (e.g. branch or tag). -func (repo *Repository) RemoveReference(name string) error { +// removeReference removes the given reference (e.g. branch or tag). +func (repo *Repository) removeReference(name string) error { return repo.gogitRepo.Storer.RemoveReference(plumbing.ReferenceName(name)) } diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index 3ead3e22165f4..8d9c54f6a87b1 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -56,8 +56,8 @@ func (repo *Repository) SetReference(name, commitID string) error { return err } -// RemoveReference removes the given reference (e.g. branch or tag). -func (repo *Repository) RemoveReference(name string) error { +// removeReference removes the given reference (e.g. branch or tag). +func (repo *Repository) removeReference(name string) error { _, _, err := NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(name).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path}) return err } diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 35636fae1a712..951936cca317b 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -116,7 +116,7 @@ func TestReadWritePullHead(t *testing.T) { assert.Equal(t, headContents, newCommit) // Remove file after the test - err = repo.RemoveReference(PullPrefix + "1/head") + err = repo.removeReference(PullPrefix + "1/head") assert.NoError(t, err) } diff --git a/modules/gitrepo/reference.go b/modules/gitrepo/reference.go new file mode 100644 index 0000000000000..70a55613cc488 --- /dev/null +++ b/modules/gitrepo/reference.go @@ -0,0 +1,17 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "context" + + "code.gitea.io/gitea/modules/git" +) + +func RemoveReference(ctx context.Context, repo Repository, refName string) error { + _, _, err := git.NewCommand("update-ref", "--no-deref", "-d"). + AddDynamicArguments(refName). + RunStdString(ctx, &git.RunOpts{Dir: repoPath(repo)}) + return err +} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index d4a5872fd1c27..b11e889eb502e 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -979,7 +979,7 @@ func DeleteIssue(ctx *context.APIContext) { return } - if err = issue_service.DeleteIssue(ctx, ctx.Doer, ctx.Repo.GitRepo, issue); err != nil { + if err = issue_service.DeleteIssue(ctx, ctx.Doer, issue); err != nil { ctx.APIErrorInternal(err) return } diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index fd34422cfcc65..a11d35da1eb85 100644 --- a/routers/web/repo/issue_list.go +++ b/routers/web/repo/issue_list.go @@ -398,7 +398,7 @@ func BatchDeleteIssues(ctx *context.Context) { return } for _, issue := range issues { - if err := issue_service.DeleteIssue(ctx, ctx.Doer, ctx.Repo.GitRepo, issue); err != nil { + if err := issue_service.DeleteIssue(ctx, ctx.Doer, issue); err != nil { ctx.ServerError("DeleteIssue", err) return } diff --git a/routers/web/repo/issue_new.go b/routers/web/repo/issue_new.go index 887019b146b89..1393f62fa531a 100644 --- a/routers/web/repo/issue_new.go +++ b/routers/web/repo/issue_new.go @@ -216,7 +216,7 @@ func DeleteIssue(ctx *context.Context) { return } - if err := issue_service.DeleteIssue(ctx, ctx.Doer, ctx.Repo.GitRepo, issue); err != nil { + if err := issue_service.DeleteIssue(ctx, ctx.Doer, issue); err != nil { ctx.ServerError("DeleteIssueByID", err) return } diff --git a/services/issue/issue.go b/services/issue/issue.go index ae81b9f48b6d0..77fb1cbcd498f 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -17,6 +17,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" notify_service "code.gitea.io/gitea/services/notify" @@ -180,7 +181,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee } // DeleteIssue deletes an issue -func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue) error { +func DeleteIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) error { // load issue before deleting it if err := issue.LoadAttributes(ctx); err != nil { return err @@ -199,8 +200,8 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } // delete pull request related git data - if issue.IsPull && gitRepo != nil { - if err := gitRepo.RemoveReference(issue.PullRequest.GetGitHeadRefName()); err != nil { + if issue.IsPull { + if err := gitrepo.RemoveReference(ctx, issue.Repo, issue.PullRequest.GetGitHeadRefName()); err != nil { return err } } diff --git a/services/pull/pull.go b/services/pull/pull.go index 2bb0582c293b5..5fea26d5272ee 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -104,13 +104,6 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { assigneeCommentMap := make(map[int64]*issues_model.Comment) - // add first push codes comment - baseGitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - return err - } - defer baseGitRepo.Close() - var reviewNotifiers []*issue_service.ReviewRequestNotifier if err := db.WithTx(ctx, func(ctx context.Context) error { if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { @@ -162,12 +155,11 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return nil }); err != nil { // cleanup: this will only remove the reference, the real commit will be clean up when next GC - if err1 := baseGitRepo.RemoveReference(pr.GetGitHeadRefName()); err1 != nil { + if err1 := gitrepo.RemoveReference(ctx, pr.BaseRepo, pr.GetGitHeadRefName()); err1 != nil { log.Error("RemoveReference: %v", err1) } return err } - baseGitRepo.Close() // close immediately to avoid notifications will open the repository again issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) From 4149db840f34400e3b907e76223ec9bbc09f5927 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 11 Sep 2025 22:41:47 -0700 Subject: [PATCH 08/12] remove unnecessary change --- tests/integration/git_misc_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/git_misc_test.go b/tests/integration/git_misc_test.go index 3d3e03645c095..eb039c7c1c9cf 100644 --- a/tests/integration/git_misc_test.go +++ b/tests/integration/git_misc_test.go @@ -171,7 +171,6 @@ func TestAgitReviewStaleness(t *testing.T) { // Get initial commit ID for the review initialCommitID := pr.HeadCommitID - assert.NoError(t, err) t.Logf("Initial commit ID: %s", initialCommitID) // Create a review on the PR (as user1 reviewing user2's PR) From 6383bca6e2d6d55b0352b6a94059baf3c93d054a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Sep 2025 12:20:45 -0700 Subject: [PATCH 09/12] Fix bug --- services/pull/pull.go | 3 ++- tests/integration/pull_update_test.go | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 5fea26d5272ee..4be888a011313 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -597,7 +597,8 @@ func UpdatePullRequestGithubFlowHead(ctx context.Context, pr *issues_model.PullR _, _, err := git.NewCommand("fetch", "--no-tags", "--refmap="). AddDynamicArguments(pr.HeadRepo.RepoPath()). - AddDynamicArguments(fmt.Sprintf("refs/heads/%s:%s", pr.HeadBranch, pr.GetGitHeadRefName())). + // + means force fetch + AddDynamicArguments(fmt.Sprintf("+refs/heads/%s:%s", pr.HeadBranch, pr.GetGitHeadRefName())). RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) return err } diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index f4565d8c9c6a2..64cb1c799d1b6 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -71,6 +71,9 @@ func TestAPIPullUpdateByRebase(t *testing.T) { AddTokenAuth(token) session.MakeRequest(t, req, http.StatusOK) + // reload pr + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + // Test GetDiverging after update diffCount, err = pull_service.GetDiverging(t.Context(), pr) assert.NoError(t, err) From d4ed6100cdbd26cc7638b187d563a397f91c508a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Sep 2025 19:49:33 -0700 Subject: [PATCH 10/12] Fix bug --- services/repository/branch.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/services/repository/branch.go b/services/repository/branch.go index 1a34121d9906b..11b78ceb53213 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -716,6 +716,16 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo return info, nil } + // we need fetch the necessary commits from the head repo first if it's not the same repository + if baseRepo.ID != headRepo.ID { + if _, _, err := git.NewCommand("fetch", "--no-tags"). + AddDynamicArguments(headRepo.RepoPath()). + AddDynamicArguments(headGitBranch.CommitID). + RunStdString(ctx, &git.RunOpts{Dir: baseRepo.RepoPath()}); err != nil { + return nil, err + } + } + // 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 From c33886fd7ad1931648fd6dd7410217ee837fe33c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 26 Sep 2025 10:03:24 -0700 Subject: [PATCH 11/12] improvements --- models/issues/pull.go | 6 ++++++ models/migrations/v1_12/v136.go | 21 ++++++++------------- modules/gitrepo/fetch.go | 29 +++++++++++++++++++++++++++++ services/pull/pull.go | 23 +++++++++-------------- services/repository/branch.go | 7 ++----- 5 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 modules/gitrepo/fetch.go diff --git a/models/issues/pull.go b/models/issues/pull.go index fb7dff3cc9e83..872163e11a8f7 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -256,6 +256,9 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { return fmt.Errorf("pr[%d].LoadHeadRepo[%d]: %w", pr.ID, pr.HeadRepoID, err) } pr.isHeadRepoLoaded = true + if pr.IsSameRepo() && pr.BaseRepo == nil { + pr.BaseRepo = pr.HeadRepo + } } return nil } @@ -322,6 +325,9 @@ func (pr *PullRequest) LoadBaseRepo(ctx context.Context) (err error) { if err != nil { return fmt.Errorf("pr[%d].LoadBaseRepo[%d]: %w", pr.ID, pr.BaseRepoID, err) } + if pr.IsSameRepo() && pr.HeadRepo == nil { + pr.HeadRepo = pr.BaseRepo + } return nil } diff --git a/models/migrations/v1_12/v136.go b/models/migrations/v1_12/v136.go index 91fca2dcc7998..20b892b6cc547 100644 --- a/models/migrations/v1_12/v136.go +++ b/models/migrations/v1_12/v136.go @@ -6,7 +6,6 @@ package v1_12 import ( "fmt" "math" - "strings" "time" repo_model "code.gitea.io/gitea/models/repo" @@ -18,19 +17,15 @@ import ( "xorm.io/xorm" ) -type Repository struct { - ID int64 `xorm:"pk autoincr"` - OwnerID int64 `xorm:"UNIQUE(s) index"` - OwnerName string - LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"` - Name string `xorm:"INDEX NOT NULL"` -} - -func (r *Repository) RelativePath() string { - return fmt.Sprintf("%s/%s.git", strings.ToLower(r.OwnerName), strings.ToLower(r.Name)) -} - func AddCommitDivergenceToPulls(x *xorm.Engine) error { + type Repository struct { + ID int64 `xorm:"pk autoincr"` + OwnerID int64 `xorm:"UNIQUE(s) index"` + OwnerName string + LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"` + Name string `xorm:"INDEX NOT NULL"` + } + type PullRequest struct { ID int64 `xorm:"pk autoincr"` IssueID int64 `xorm:"INDEX"` diff --git a/modules/gitrepo/fetch.go b/modules/gitrepo/fetch.go new file mode 100644 index 0000000000000..2611a918062a6 --- /dev/null +++ b/modules/gitrepo/fetch.go @@ -0,0 +1,29 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "context" + "fmt" + + "code.gitea.io/gitea/modules/git/gitcmd" +) + +// FetchRemoteBranch fetches a remote branch into a local branch +func FetchRemoteBranch(ctx context.Context, repo Repository, localBranch string, remoteRepo Repository, remoteBranch string, args ...string) error { + _, _, err := gitcmd.NewCommand("fetch", "--no-tags", "--refmap="). + AddDynamicArguments(repoPath(remoteRepo)). + // + means force fetch + AddDynamicArguments(fmt.Sprintf("+refs/heads/%s:%s", remoteBranch, localBranch)). + RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath(repo)}) + return err +} + +func FetchRemoteCommit(ctx context.Context, repo, remoteRepo Repository, commitID string) error { + _, _, err := gitcmd.NewCommand("fetch", "--no-tags"). + AddDynamicArguments(repoPath(remoteRepo)). + AddDynamicArguments(commitID). + RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath(repo)}) + return err +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 57958b907104d..f51f85e7aadba 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -560,8 +560,7 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r func UpdatePullRequestAgitFlowHead(ctx context.Context, pr *issues_model.PullRequest, commitID string) error { log.Trace("UpdateAgitPullRequestHead[%d]: update pull request head in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) - _, _, err := gitcmd.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), commitID).RunStdString(ctx, &gitcmd.RunOpts{Dir: pr.BaseRepo.RepoPath()}) - return err + return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), commitID) } // UpdatePullRequestHeadRef updates the head reference of a pull request @@ -572,21 +571,17 @@ func UpdatePullRequestGithubFlowHead(ctx context.Context, pr *issues_model.PullR return err } - if err := gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadCommitID); err != nil { - log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) - } + if !pr.IsSameRepo() { // for cross repository pull request + if err := pr.LoadHeadRepo(ctx); err != nil { + return err + } - // for cross repository pull request - if err := pr.LoadHeadRepo(ctx); err != nil { - return err + if err := gitrepo.FetchRemoteBranch(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadRepo, pr.HeadBranch); err != nil { + return err + } } - _, _, err := gitcmd.NewCommand("fetch", "--no-tags", "--refmap="). - AddDynamicArguments(pr.HeadRepo.RepoPath()). - // + means force fetch - AddDynamicArguments(fmt.Sprintf("+refs/heads/%s:%s", pr.HeadBranch, pr.GetGitHeadRefName())). - RunStdString(ctx, &gitcmd.RunOpts{Dir: pr.BaseRepo.RepoPath()}) - return err + return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadCommitID) } // retargetBranchPulls change target branch for all pull requests whose base branch is the branch diff --git a/services/repository/branch.go b/services/repository/branch.go index 6c10210d5b649..a8a0c8b0f076d 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -19,7 +19,6 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/json" @@ -719,10 +718,8 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo // we need fetch the necessary commits from the head repo first if it's not the same repository if baseRepo.ID != headRepo.ID { - if _, _, err := gitcmd.NewCommand("fetch", "--no-tags"). - AddDynamicArguments(headRepo.RepoPath()). - AddDynamicArguments(headGitBranch.CommitID). - RunStdString(ctx, &gitcmd.RunOpts{Dir: baseRepo.RepoPath()}); err != nil { + // git default will gc the commit in 2 weeks, so it's safe to do the compare + if err := gitrepo.FetchRemoteCommit(ctx, baseRepo, headRepo, headGitBranch.CommitID); err != nil { return nil, err } } From 1cf2e663ae457bac40bcb9bb71cc4b193e93f9c7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 26 Sep 2025 10:10:07 -0700 Subject: [PATCH 12/12] improvements --- services/agit/agit.go | 2 +- services/pull/pull.go | 32 +++++++++++++------------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/services/agit/agit.go b/services/agit/agit.go index 203f1e38af3cb..c5bd78bcd4d7b 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -216,7 +216,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } pr.HeadCommitID = opts.NewCommitIDs[i] - if err = pull_service.UpdatePullRequestAgitFlowHead(ctx, pr, pr.HeadCommitID); err != nil { + if err = pull_service.UpdatePullRequestHeadRef(ctx, pr); err != nil { return nil, fmt.Errorf("failed to update pull ref. Error: %w", err) } diff --git a/services/pull/pull.go b/services/pull/pull.go index f51f85e7aadba..ad271d2912acd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -123,12 +123,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { issue.PullRequest = pr // update head commit id into git repository - if pr.Flow == issues_model.PullRequestFlowAGit { - err = UpdatePullRequestAgitFlowHead(ctx, pr, pr.HeadCommitID) - } else { - err = UpdatePullRequestGithubFlowHead(ctx, pr) - } - if err != nil { + if err = UpdatePullRequestHeadRef(ctx, pr); err != nil { return err } @@ -388,7 +383,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Trace("Updating PR[%d]: composing new test task", pr.ID) pr.HeadRepo = repo // avoid loading again if pr.Flow == issues_model.PullRequestFlowGithub { - if err := UpdatePullRequestGithubFlowHead(ctx, pr); err != nil { + if err := UpdatePullRequestHeadRef(ctx, pr); err != nil { log.Error("PushToBaseRepo: %v", err) continue } @@ -549,7 +544,7 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r for _, pr := range prs { log.Trace("Updating PR[%d]: composing new test task", pr.ID) if pr.Flow == issues_model.PullRequestFlowGithub { - if err := UpdatePullRequestGithubFlowHead(ctx, pr); err != nil { + if err := UpdatePullRequestHeadRef(ctx, pr); err != nil { log.Error("UpdatePullRequestHead: %v", err) } } @@ -557,31 +552,30 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r } } -func UpdatePullRequestAgitFlowHead(ctx context.Context, pr *issues_model.PullRequest, commitID string) error { - log.Trace("UpdateAgitPullRequestHead[%d]: update pull request head in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) - - return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), commitID) -} - // UpdatePullRequestHeadRef updates the head reference of a pull request -func UpdatePullRequestGithubFlowHead(ctx context.Context, pr *issues_model.PullRequest) error { +func UpdatePullRequestHeadRef(ctx context.Context, pr *issues_model.PullRequest) error { log.Trace("UpdatePullRequestHeadRef[%d]: update pull request ref in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) if err := pr.LoadBaseRepo(ctx); err != nil { return err } + if pr.Flow == issues_model.PullRequestFlowAGit { + if pr.HeadCommitID == "" { + return errors.New("head commit ID cannot be empty for agit flow") + } + return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadCommitID) + } + if !pr.IsSameRepo() { // for cross repository pull request if err := pr.LoadHeadRepo(ctx); err != nil { return err } - if err := gitrepo.FetchRemoteBranch(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadRepo, pr.HeadBranch); err != nil { - return err - } + return gitrepo.FetchRemoteBranch(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadRepo, pr.HeadBranch) } - return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadCommitID) + return gitrepo.UpdateRef(ctx, pr.BaseRepo, pr.GetGitHeadRefName(), pr.HeadBranch) } // retargetBranchPulls change target branch for all pull requests whose base branch is the branch