Change how merged PR commit info are prepared (#3368)

* Change how merged PR commits and diff are made

* Update code.gitea.io/git dependency

* Fix typo

* Remove unneeded local variable

models/pull.go

@@ -132,6 +132,11 @@ func (pr *PullRequest) GetDefaultSquashMessage() string {
 	return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index)
 }
 
+// GetGitRefName returns git ref for hidden pull request branch
+func (pr *PullRequest) GetGitRefName() string {
+	return fmt.Sprintf("refs/pull/%d/head", pr.Index)
+}
+
 // APIFormat assumes following fields have been assigned with valid values:
 // Required - Issue
 // Optional - Merger
@@ -562,7 +567,7 @@ func (pr *PullRequest) getMergeCommit() (*git.Commit, error) {
 	indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond()))
 	defer os.Remove(indexTmpPath)
 
-	headFile := fmt.Sprintf("refs/pull/%d/head", pr.Index)
+	headFile := pr.GetGitRefName()
 
 	// Check if a pull request is merged into BaseBranch
 	_, stderr, err := process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("isMerged (git merge-base --is-ancestor): %d", pr.BaseRepo.ID),
@@ -980,7 +985,7 @@ func (pr *PullRequest) UpdatePatch() (err error) {
 // corresponding branches of base repository.
 // FIXME: Only push branches that are actually updates?
 func (pr *PullRequest) PushToBaseRepo() (err error) {
-	log.Trace("PushToBaseRepo[%d]: pushing commits to base repo 'refs/pull/%d/head'", pr.BaseRepoID, pr.Index)
+	log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitRefName())
 
 	headRepoPath := pr.HeadRepo.RepoPath()
 	headGitRepo, err := git.OpenRepository(headRepoPath)
@@ -995,7 +1000,7 @@ func (pr *PullRequest) PushToBaseRepo() (err error) {
 	// Make sure to remove the remote even if the push fails
 	defer headGitRepo.RemoveRemote(tmpRemoteName)
 
-	headFile := fmt.Sprintf("refs/pull/%d/head", pr.Index)
+	headFile := pr.GetGitRefName()
 
 	// Remove head in case there is a conflict.
 	file := path.Join(pr.BaseRepo.RepoPath(), headFile)

routers/repo/pull.go

@@ -253,40 +253,30 @@ func setMergeTarget(ctx *context.Context, pull *models.PullRequest) {
 }
 
 // PrepareMergedViewPullInfo show meta information for a merged pull request view page
-func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) {
+func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.PullRequestInfo {
 	pull := issue.PullRequest
 
-	var err error
-	if err = pull.GetHeadRepo(); err != nil {
-		ctx.ServerError("GetHeadRepo", err)
-		return
-	}
-
 	setMergeTarget(ctx, pull)
 	ctx.Data["HasMerged"] = true
 
-	mergedCommit, err := ctx.Repo.GitRepo.GetCommit(pull.MergedCommitID)
-	if err != nil {
-		ctx.ServerError("GetCommit", err)
-		return
-	}
-	// the ID of the last commit in the PR (not including the merge commit)
-	endCommitID, err := mergedCommit.ParentID(mergedCommit.ParentCount() - 1)
-	if err != nil {
-		ctx.ServerError("ParentID", err)
-		return
-	}
+	prInfo, err := ctx.Repo.GitRepo.GetPullRequestInfo(ctx.Repo.Repository.RepoPath(),
+		pull.MergeBase, pull.GetGitRefName())
 
-	ctx.Data["NumCommits"], err = ctx.Repo.GitRepo.CommitsCountBetween(pull.MergeBase, endCommitID.String())
-	if err != nil {
-		ctx.ServerError("Repo.GitRepo.CommitsCountBetween", err)
-		return
-	}
-	ctx.Data["NumFiles"], err = ctx.Repo.GitRepo.FilesCountBetween(pull.MergeBase, endCommitID.String())
 	if err != nil {
-		ctx.ServerError("Repo.GitRepo.FilesCountBetween", err)
-		return
+		if strings.Contains(err.Error(), "fatal: Not a valid object name") {
+			ctx.Data["IsPullReuqestBroken"] = true
+			ctx.Data["BaseTarget"] = "deleted"
+			ctx.Data["NumCommits"] = 0
+			ctx.Data["NumFiles"] = 0
+			return nil
+		}
+
+		ctx.ServerError("GetPullRequestInfo", err)
+		return nil
 	}
+	ctx.Data["NumCommits"] = prInfo.Commits.Len()
+	ctx.Data["NumFiles"] = prInfo.NumFiles
+	return prInfo
 }
 
 // PrepareViewPullInfo show meta information for a pull request preview page
@@ -351,28 +341,16 @@ func ViewPullCommits(ctx *context.Context) {
 
 	var commits *list.List
 	if pull.HasMerged {
-		PrepareMergedViewPullInfo(ctx, issue)
+		prInfo := PrepareMergedViewPullInfo(ctx, issue)
 		if ctx.Written() {
 			return
+		} else if prInfo == nil {
+			ctx.NotFound("ViewPullCommits", nil)
+			return
 		}
 		ctx.Data["Username"] = ctx.Repo.Owner.Name
 		ctx.Data["Reponame"] = ctx.Repo.Repository.Name
-
-		mergedCommit, err := ctx.Repo.GitRepo.GetCommit(pull.MergedCommitID)
-		if err != nil {
-			ctx.ServerError("Repo.GitRepo.GetCommit", err)
-			return
-		}
-		endCommitID, err := mergedCommit.ParentID(mergedCommit.ParentCount() - 1)
-		if err != nil {
-			ctx.ServerError("ParentID", err)
-			return
-		}
-		commits, err = ctx.Repo.GitRepo.CommitsBetweenIDs(endCommitID.String(), pull.MergeBase)
-		if err != nil {
-			ctx.ServerError("Repo.GitRepo.CommitsBetweenIDs", err)
-			return
-		}
+		commits = prInfo.Commits
 	} else {
 		prInfo := PrepareViewPullInfo(ctx, issue)
 		if ctx.Written() {
@@ -415,25 +393,25 @@ func ViewPullFiles(ctx *context.Context) {
 
 	var headTarget string
 	if pull.HasMerged {
-		PrepareMergedViewPullInfo(ctx, issue)
+		prInfo := PrepareMergedViewPullInfo(ctx, issue)
 		if ctx.Written() {
 			return
+		} else if prInfo == nil {
+			ctx.NotFound("ViewPullFiles", nil)
+			return
 		}
 
 		diffRepoPath = ctx.Repo.GitRepo.Path
-		startCommitID = pull.MergeBase
-		mergedCommit, err := ctx.Repo.GitRepo.GetCommit(pull.MergedCommitID)
-		if err != nil {
-			ctx.ServerError("GetCommit", err)
-			return
-		}
-		endCommitSha, err := mergedCommit.ParentID(mergedCommit.ParentCount() - 1)
+		gitRepo = ctx.Repo.GitRepo
+
+		headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName())
 		if err != nil {
-			ctx.ServerError("ParentID", err)
+			ctx.ServerError("GetRefCommitID", err)
 			return
 		}
-		endCommitID = endCommitSha.String()
-		gitRepo = ctx.Repo.GitRepo
+
+		startCommitID = prInfo.MergeBase
+		endCommitID = headCommitID
 
 		headTarget = path.Join(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
 		ctx.Data["Username"] = ctx.Repo.Owner.Name

vendor/code.gitea.io/git/repo_commit.go

@@ -11,8 +11,8 @@ import (
 	"strings"
 )
 
-// getRefCommitID returns the last commit ID string of given reference (branch or tag).
-func (repo *Repository) getRefCommitID(name string) (string, error) {
+// GetRefCommitID returns the last commit ID string of given reference (branch or tag).
+func (repo *Repository) GetRefCommitID(name string) (string, error) {
 	stdout, err := NewCommand("show-ref", "--verify", name).RunInDir(repo.Path)
 	if err != nil {
 		if strings.Contains(err.Error(), "not a valid ref") {
@@ -25,12 +25,12 @@ func (repo *Repository) getRefCommitID(name string) (string, error) {
 
 // GetBranchCommitID returns last commit ID string of given branch.
 func (repo *Repository) GetBranchCommitID(name string) (string, error) {
-	return repo.getRefCommitID(BranchPrefix + name)
+	return repo.GetRefCommitID(BranchPrefix + name)
 }
 
 // GetTagCommitID returns last commit ID string of given tag.
 func (repo *Repository) GetTagCommitID(name string) (string, error) {
-	return repo.getRefCommitID(TagPrefix + name)
+	return repo.GetRefCommitID(TagPrefix + name)
 }
 
 // parseCommitData parses commit information from the (uncompressed) raw

vendor/vendor.json

@@ -3,10 +3,10 @@
 	"ignore": "test appengine",
 	"package": [
 		{
-			"checksumSHA1": "1WHdGmDRsFRTD5N69l+MEbZr+nM=",
+			"checksumSHA1": "Gz+a5Qo4PCiB/Gf2f02v8HEAxDM=",
 			"path": "code.gitea.io/git",
-			"revision": "f4a91053671bee69f1995e456c1541668717c19d",
-			"revisionTime": "2018-01-07T06:11:05Z"
+			"revision": "6798d0f202cdc7187c00a467b586a4bdee27e8c9",
+			"revisionTime": "2018-01-14T14:37:32Z"
 		},
 		{
 			"checksumSHA1": "Qtq0kW+BnpYMOriaoCjMa86WGG8=",