From 40ec270e6959277c52ab56228be4ab7561494e2f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 8 Sep 2025 18:59:26 -0700 Subject: [PATCH 1/3] rework for calculating head branch of migrating pull requests --- models/issues/pull.go | 1 - routers/web/repo/issue_comment.go | 1 - routers/web/repo/pull_review.go | 9 +- routers/web/repo/pull_review_test.go | 2 +- services/agit/agit.go | 64 ++++--- services/migrations/gitea_uploader.go | 187 +++++---------------- services/migrations/gitea_uploader_test.go | 2 +- services/pull/patch.go | 5 +- services/pull/pull.go | 31 ++-- services/pull/temp_repo.go | 6 +- tests/integration/git_misc_test.go | 3 +- 11 files changed, 101 insertions(+), 210 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..e3dec14374560 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 update a review but could not get head commit SHA: %v", err) + ctx.Resp.WriteHeader(http.StatusBadRequest) + return + } } updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files)) @@ -332,7 +337,7 @@ func UpdateViewedFiles(ctx *context.Context) { } if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil { - ctx.ServerError("UpdateReview", err) + ctx.ServerError("UpdateReviewState", err) } } diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 42223c1d9c616..31e1993f6640a 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -41,7 +41,7 @@ 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) + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadBranch, nil) require.NoError(t, err) comment.Invalidated = true diff --git a/services/agit/agit.go b/services/agit/agit.go index 63b3eab4f2d4e..dfadc253721d0 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.UpdateRef(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/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 75eb06d01fa3f..8d681d1a8ab44 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -8,8 +8,6 @@ import ( "context" "fmt" "io" - "os" - "path/filepath" "strconv" "strings" "time" @@ -31,7 +29,7 @@ import ( "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/uri" "code.gitea.io/gitea/modules/util" - "code.gitea.io/gitea/services/pull" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" "github.com/google/uuid" @@ -554,164 +552,65 @@ func (g *GiteaLocalUploader) CreatePullRequests(ctx context.Context, prs ...*bas if err := issues_model.InsertPullRequests(ctx, gprs...); err != nil { return err } - for _, pr := range gprs { + for i, pr := range gprs { g.issues[pr.Issue.Index] = pr.Issue - pull.StartPullRequestCheckImmediately(ctx, pr) + // create head commit ref immediately, return error if failed. + if err := pull_service.UpdateRef(ctx, pr, prs[i].Head.SHA); err != nil { + return err + } + + pull_service.StartPullRequestCheckImmediately(ctx, pr) } return nil } -func (g *GiteaLocalUploader) updateGitForPullRequest(ctx context.Context, pr *base.PullRequest) (head string, err error) { - // SECURITY: this pr must have been must have been ensured safe +func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(pr *base.PullRequest) (head string, err error) { + // SECURITY: this pr must have been ensured safe if !pr.EnsuredSafe { log.Error("PR #%d in %s/%s has not been checked for safety.", pr.Number, g.repoOwner, g.repoName) return "", fmt.Errorf("the PR[%d] was not checked for safety", pr.Number) } - // Anonymous function to download the patch file (allows us to use defer) - err = func() error { - // if the patchURL is empty there is nothing to download - if pr.PatchURL == "" { - return nil - } - - // SECURITY: We will assume that the pr.PatchURL has been checked - // pr.PatchURL maybe a local file - but note EnsureSafe should be asserting that this safe - ret, err := uri.Open(pr.PatchURL) // TODO: This probably needs to use the downloader as there may be rate limiting issues here - if err != nil { - return err - } - defer ret.Close() - - pullDir := filepath.Join(g.repo.RepoPath(), "pulls") - if err = os.MkdirAll(pullDir, os.ModePerm); err != nil { - return err - } - - f, err := os.Create(filepath.Join(pullDir, fmt.Sprintf("%d.patch", pr.Number))) - if err != nil { - return err - } - defer f.Close() - - // TODO: Should there be limits on the size of this file? - _, err = io.Copy(f, ret) - - return err - }() - if err != nil { - return "", err - } - - head = "unknown repository" - if pr.IsForkPullRequest() && pr.State != "closed" { - // OK we want to fetch the current head as a branch from its CloneURL - - // 1. Is there a head clone URL available? - // 2. Is there a head ref available? - if pr.Head.CloneURL == "" || pr.Head.Ref == "" { - return head, nil + if pr.IsForkPullRequest() { + if pr.Head.SHA == "" { + return "", fmt.Errorf("the PR[%d] does not have a head commit SHA", pr.Number) } - - // 3. We need to create a remote for this clone url - // ... maybe we already have a name for this remote - remote, ok := g.prHeadCache[pr.Head.CloneURL+":"] - if !ok { - // ... let's try ownername as a reasonable name - remote = pr.Head.OwnerName - if !git.IsValidRefPattern(remote) { - // ... let's try something less nice - remote = "head-pr-" + strconv.FormatInt(pr.Number, 10) - } - // ... now add the remote - err := g.gitRepo.AddRemote(remote, pr.Head.CloneURL, true) - if err != nil { - log.Error("PR #%d in %s/%s AddRemote[%s] failed: %v", pr.Number, g.repoOwner, g.repoName, remote, err) - } else { - g.prHeadCache[pr.Head.CloneURL+":"] = remote - ok = true + // ignore the original branch name because it belongs to the head repository + headBranch := fmt.Sprintf("branch_for_pr_%d", pr.Number) + if pr.State != "closed" { + // create the head branch + if err := g.gitRepo.CreateBranch(headBranch, pr.Head.SHA); err != nil { + return "", fmt.Errorf("failed to create head branch[%s] for PR[%d]: %w", headBranch, pr.Number, err) } } - if !ok { - return head, nil - } + return headBranch, nil // assign a non-exist branch + } - // 4. Check if we already have this ref? - localRef, ok := g.prHeadCache[pr.Head.CloneURL+":"+pr.Head.Ref] - if !ok { - // ... We would normally name this migrated branch as / but we need to ensure that is safe - localRef = git.SanitizeRefPattern(pr.Head.OwnerName + "/" + pr.Head.Ref) - - // ... Now we must assert that this does not exist - if g.gitRepo.IsBranchExist(localRef) { - localRef = "head-pr-" + strconv.FormatInt(pr.Number, 10) + "/" + localRef - i := 0 - for g.gitRepo.IsBranchExist(localRef) { - if i > 5 { - // ... We tried, we really tried but this is just a seriously unfriendly repo - return head, nil - } - // OK just try some uuids! - localRef = git.SanitizeRefPattern("head-pr-" + strconv.FormatInt(pr.Number, 10) + uuid.New().String()) - i++ + if pr.Head.SHA != "" { + if pr.Head.Ref == "" { + headBranch := fmt.Sprintf("branch_for_pr_%d", pr.Number) + if pr.State != "closed" { + // create the head branch + if err := g.gitRepo.CreateBranch(headBranch, pr.Head.SHA); err != nil { + return "", fmt.Errorf("failed to create head branch[%s] for PR[%d]: %w", headBranch, pr.Number, err) } } - - fetchArg := pr.Head.Ref + ":" + git.BranchPrefix + localRef - if strings.HasPrefix(fetchArg, "-") { - fetchArg = git.BranchPrefix + fetchArg - } - - _, _, err = git.NewCommand("fetch", "--no-tags").AddDashesAndList(remote, fetchArg).RunStdString(ctx, &git.RunOpts{Dir: g.repo.RepoPath()}) - if err != nil { - log.Error("Fetch branch from %s failed: %v", pr.Head.CloneURL, err) - return head, nil - } - g.prHeadCache[pr.Head.CloneURL+":"+pr.Head.Ref] = localRef - head = localRef - } - - // 5. Now if pr.Head.SHA == "" we should recover this to the head of this branch - if pr.Head.SHA == "" { - headSha, err := g.gitRepo.GetBranchCommitID(localRef) - if err != nil { - log.Error("unable to get head SHA of local head for PR #%d from %s in %s/%s. Error: %v", pr.Number, pr.Head.Ref, g.repoOwner, g.repoName, err) - return head, nil - } - pr.Head.SHA = headSha - } - - _, _, err = git.NewCommand("update-ref", "--no-deref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.Head.SHA).RunStdString(ctx, &git.RunOpts{Dir: g.repo.RepoPath()}) - if err != nil { - return "", err + return headBranch, nil // assign a non-exist branch } - - return head, nil + // it's unnecessary to check whether the ref exists because the git repository will be mirrored cloned. + return pr.Head.Ref, nil } - if pr.Head.Ref != "" { - head = pr.Head.Ref + if pr.Head.Ref == "" { + return "", fmt.Errorf("the PR[%d] does not have a head commit SHA or ref", pr.Number) } - // Ensure the closed PR SHA still points to an existing ref - if pr.Head.SHA == "" { - // The SHA is empty - log.Warn("Empty reference, no pull head for PR #%d in %s/%s", pr.Number, g.repoOwner, g.repoName) - } else { - _, _, err = git.NewCommand("rev-list", "--quiet", "-1").AddDynamicArguments(pr.Head.SHA).RunStdString(ctx, &git.RunOpts{Dir: g.repo.RepoPath()}) - if err != nil { - // Git update-ref remove bad references with a relative path - log.Warn("Deprecated local head %s for PR #%d in %s/%s, removing %s", pr.Head.SHA, pr.Number, g.repoOwner, g.repoName, pr.GetGitHeadRefName()) - } else { - // set head information - _, _, err = git.NewCommand("update-ref", "--no-deref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.Head.SHA).RunStdString(ctx, &git.RunOpts{Dir: g.repo.RepoPath()}) - if err != nil { - log.Error("unable to set %s as the local head for PR #%d from %s in %s/%s. Error: %v", pr.Head.SHA, pr.Number, pr.Head.Ref, g.repoOwner, g.repoName, err) - } - } + // if there is no sha, we need to get the sha from the head branch + pr.Head.SHA, err = g.gitRepo.GetRefCommitID(pr.Head.Ref) + if err != nil { + return "", fmt.Errorf("the PR[%d] head ref[%s] is invalid: %w", pr.Number, pr.Head.Ref, err) } - - return head, nil + return pr.Head.Ref, nil } func (g *GiteaLocalUploader) newPullRequest(ctx context.Context, pr *base.PullRequest) (*issues_model.PullRequest, error) { @@ -725,9 +624,10 @@ func (g *GiteaLocalUploader) newPullRequest(ctx context.Context, pr *base.PullRe milestoneID := g.milestones[pr.Milestone] - head, err := g.updateGitForPullRequest(ctx, pr) + // recalculate and create head branch when necessary + headBranch, err := g.updateHeadBranchForPullRequest(pr) if err != nil { - return nil, fmt.Errorf("updateGitForPullRequest: %w", err) + return nil, fmt.Errorf("updateHeadBranchForPullRequest: %w", err) } // Now we may need to fix the mergebase @@ -795,14 +695,13 @@ func (g *GiteaLocalUploader) newPullRequest(ctx context.Context, pr *base.PullRe pullRequest := issues_model.PullRequest{ HeadRepoID: g.repo.ID, - HeadBranch: head, + HeadBranch: headBranch, BaseRepoID: g.repo.ID, BaseBranch: pr.Base.Ref, MergeBase: pr.Base.SHA, Index: pr.Number, HasMerged: pr.Merged, - - Issue: &issue, + Issue: &issue, } if pullRequest.Issue.IsClosed && pr.Closed != nil { diff --git a/services/migrations/gitea_uploader_test.go b/services/migrations/gitea_uploader_test.go index 4856b66c6e157..6a45646f60c3e 100644 --- a/services/migrations/gitea_uploader_test.go +++ b/services/migrations/gitea_uploader_test.go @@ -507,7 +507,7 @@ func TestGiteaUploadUpdateGitForPullRequest(t *testing.T) { testCase.pr.EnsuredSafe = true - head, err := uploader.updateGitForPullRequest(ctx, &testCase.pr) + head, err := uploader.updateHeadBranchForPullRequest(&testCase.pr) assert.NoError(t, err) assert.Equal(t, testCase.head, head) 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 a369e64dbe08f..b4285302b61ce 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -47,6 +47,7 @@ type NewPullRequestOptions struct { LabelIDs []int64 AttachmentUUIDs []string PullRequest *issues_model.PullRequest + HeadCommitID string // only for agit flow. For github flow, it will use head branch directly AssigneeIDs []int64 Reviewers []*user_model.User TeamReviewers []*organization.Team @@ -132,10 +133,11 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { pr.Issue = issue issue.PullRequest = pr + // update head commit ID if pr.Flow == issues_model.PullRequestFlowGithub { err = PushToBaseRepo(ctx, pr) } else { - err = UpdateRef(ctx, pr) + err = UpdateRef(ctx, pr, opts.HeadCommitID) } if err != nil { return err @@ -639,15 +641,15 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r } } -// UpdateRef update refs/pull/id/head directly for agit flow pull request -func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { +// UpdateRef update refs/pull/id/head directly for agit flow pull request or the github pull request with the same base/head repository +func UpdateRef(ctx context.Context, pr *issues_model.PullRequest, headCommitID string) (err error) { log.Trace("UpdateRef[%d]: upgate 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()}) + _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), 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) } @@ -811,19 +813,14 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } defer closer.Close() - var headCommit *git.Commit - if pr.Flow == issues_model.PullRequestFlowGithub { - headCommit, err = gitRepo.GetBranchCommit(pr.HeadBranch) - } else { - 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(pr.HeadCommitID) + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err != nil { + log.Error("Unable to get head commit id: %s Error: %v", pr.GetGitHeadRefName(), err) + return "" } + headCommit, err := gitRepo.GetCommit(headCommitID) if err != nil { - log.Error("Unable to get head commit: %s Error: %v", pr.HeadBranch, err) + log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) return "" } @@ -1042,11 +1039,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..7fe505b09a855 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -1,5 +1,4 @@ -// Copyright 2019 The Gitea Authors. -// All rights reserved. +// Copyright 2019 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package pull @@ -166,13 +165,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 1a8402e9470e00e9e5d912bd099a04005270022a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 8 Sep 2025 20:04:58 -0700 Subject: [PATCH 2/3] improvements --- services/migrations/gitea_uploader.go | 22 +++++++++++++++------- services/migrations/gitea_uploader_test.go | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 8d681d1a8ab44..9c9f72321c474 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -564,7 +565,7 @@ func (g *GiteaLocalUploader) CreatePullRequests(ctx context.Context, prs ...*bas return nil } -func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(pr *base.PullRequest) (head string, err error) { +func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(ctx context.Context, pr *base.PullRequest) (head string, err error) { // SECURITY: this pr must have been ensured safe if !pr.EnsuredSafe { log.Error("PR #%d in %s/%s has not been checked for safety.", pr.Number, g.repoOwner, g.repoName) @@ -576,14 +577,21 @@ func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(pr *base.PullRequest return "", fmt.Errorf("the PR[%d] does not have a head commit SHA", pr.Number) } // ignore the original branch name because it belongs to the head repository - headBranch := fmt.Sprintf("branch_for_pr_%d", pr.Number) + headBranch := fmt.Sprintf("%s-%s", pr.Head.OwnerName, pr.Head.Ref) if pr.State != "closed" { + exist, err := git_model.IsBranchExist(ctx, g.repo.ID, headBranch) + if err != nil { + return "", fmt.Errorf("failed to check if head branch[%s] exists for PR[%d]: %w", headBranch, pr.Number, err) + } + if exist { + headBranch = fmt.Sprintf("%s-%s-%d", pr.Head.OwnerName, pr.Head.Ref, pr.Number) + } // create the head branch - if err := g.gitRepo.CreateBranch(headBranch, pr.Head.SHA); err != nil { + if err := repo_service.CreateNewBranchFromCommit(ctx, g.doer, g.repo, g.gitRepo, pr.Head.SHA, headBranch); err != nil { return "", fmt.Errorf("failed to create head branch[%s] for PR[%d]: %w", headBranch, pr.Number, err) } } - return headBranch, nil // assign a non-exist branch + return headBranch, nil } if pr.Head.SHA != "" { @@ -591,7 +599,7 @@ func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(pr *base.PullRequest headBranch := fmt.Sprintf("branch_for_pr_%d", pr.Number) if pr.State != "closed" { // create the head branch - if err := g.gitRepo.CreateBranch(headBranch, pr.Head.SHA); err != nil { + if err := repo_service.CreateNewBranchFromCommit(ctx, g.doer, g.repo, g.gitRepo, pr.Head.SHA, headBranch); err != nil { return "", fmt.Errorf("failed to create head branch[%s] for PR[%d]: %w", headBranch, pr.Number, err) } } @@ -601,7 +609,7 @@ func (g *GiteaLocalUploader) updateHeadBranchForPullRequest(pr *base.PullRequest return pr.Head.Ref, nil } - if pr.Head.Ref == "" { + if pr.Head.Ref == "" { // both sha and ref are empty return "", fmt.Errorf("the PR[%d] does not have a head commit SHA or ref", pr.Number) } @@ -625,7 +633,7 @@ func (g *GiteaLocalUploader) newPullRequest(ctx context.Context, pr *base.PullRe milestoneID := g.milestones[pr.Milestone] // recalculate and create head branch when necessary - headBranch, err := g.updateHeadBranchForPullRequest(pr) + headBranch, err := g.updateHeadBranchForPullRequest(ctx, pr) if err != nil { return nil, fmt.Errorf("updateHeadBranchForPullRequest: %w", err) } diff --git a/services/migrations/gitea_uploader_test.go b/services/migrations/gitea_uploader_test.go index 6a45646f60c3e..c0ac5ccb7ebe7 100644 --- a/services/migrations/gitea_uploader_test.go +++ b/services/migrations/gitea_uploader_test.go @@ -507,7 +507,7 @@ func TestGiteaUploadUpdateGitForPullRequest(t *testing.T) { testCase.pr.EnsuredSafe = true - head, err := uploader.updateHeadBranchForPullRequest(&testCase.pr) + head, err := uploader.updateHeadBranchForPullRequest(t.Context(), &testCase.pr) assert.NoError(t, err) assert.Equal(t, testCase.head, head) From b27478b4c3ec76acd0ff790cbb65cad67c2bb161 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 8 Sep 2025 20:13:24 -0700 Subject: [PATCH 3/3] Fix test --- routers/web/repo/pull_review_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 31e1993f6640a..99ba9c3272580 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.HeadBranch, nil) + headCommitID, err := ctx.Repo.GitRepo.GetBranchCommitID(pr.HeadBranch) + 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