improve create pull request head ref

Author: lunny

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"`

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)
 				}
 

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))

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

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)

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
 	}

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
 		}
 	}

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()
 	}

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)