Various Merge Base fixes (#10786)

* Fix broken merge base migration v128 for merged PR

* Allow PRs with deleted base branches to still show diff

* as per @lunny

Co-authored-by: Lauris BH <lauris@nix.lv>

4 files changed, 148 insertions, 8 deletions

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 3f18a18c6d..49b34861d6 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -200,6 +200,8 @@ var migrations = []Migration{
 	NewMigration("Add Branch Protection Protected Files Column", addBranchProtectionProtectedFilesColumn),
 	// v133 -> v134
 	NewMigration("Add EmailHash Table", addEmailHashTable),
+	// v134 -> v135
+	NewMigration("Refix merge base for merged pull requests", refixMergeBase),
 }
 
 // Migrate database to current version
diff --git a/models/migrations/v128.go b/models/migrations/v128.go
index 557282bf28..1f4bc20527 100644
--- a/models/migrations/v128.go
+++ b/models/migrations/v128.go
@@ -53,7 +53,7 @@ func fixMergeBase(x *xorm.Engine) error {
 			break
 		}
 
-		i += 50
+		i += len(prs)
 		for _, pr := range prs {
 			baseRepo := &Repository{ID: pr.BaseRepoID}
 			has, err := x.Table("repository").Get(baseRepo)
@@ -81,10 +81,22 @@ func fixMergeBase(x *xorm.Engine) error {
 					}
 				}
 			} else {
-				var err error
-				pr.MergeBase, err = git.NewCommand("merge-base", "--", pr.MergedCommitID+"^", gitRefName).RunInDir(repoPath)
+				parentsString, err := git.NewCommand("rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunInDir(repoPath)
+				if err != nil {
+					log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
+					continue
+				}
+				parents := strings.Split(strings.TrimSpace(parentsString), " ")
+				if len(parents) < 2 {
+					continue
+				}
+
+				args := append([]string{"merge-base", "--"}, parents[1:]...)
+				args = append(args, gitRefName)
+
+				pr.MergeBase, err = git.NewCommand(args...).RunInDir(repoPath)
 				if err != nil {
-					log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
+					log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
 					continue
 				}
 			}
diff --git a/models/migrations/v134.go b/models/migrations/v134.go
new file mode 100644
index 0000000000..527cbafe07
--- /dev/null
+++ b/models/migrations/v134.go
@@ -0,0 +1,96 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+
+	"xorm.io/xorm"
+)
+
+func refixMergeBase(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"`
+		Index      int64
+		HeadRepoID int64 `xorm:"INDEX"`
+		BaseRepoID int64 `xorm:"INDEX"`
+		HeadBranch string
+		BaseBranch string
+		MergeBase  string `xorm:"VARCHAR(40)"`
+
+		HasMerged      bool   `xorm:"INDEX"`
+		MergedCommitID string `xorm:"VARCHAR(40)"`
+	}
+
+	var limit = setting.Database.IterateBufferSize
+	if limit <= 0 {
+		limit = 50
+	}
+
+	i := 0
+	for {
+		prs := make([]PullRequest, 0, 50)
+		if err := x.Limit(limit, i).Asc("id").Where("has_merged = ?", true).Find(&prs); err != nil {
+			return fmt.Errorf("Find: %v", err)
+		}
+		if len(prs) == 0 {
+			break
+		}
+
+		i += len(prs)
+		for _, pr := range prs {
+			baseRepo := &Repository{ID: pr.BaseRepoID}
+			has, err := x.Table("repository").Get(baseRepo)
+			if err != nil {
+				return fmt.Errorf("Unable to get base repo %d %v", pr.BaseRepoID, err)
+			}
+			if !has {
+				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)
+
+			parentsString, err := git.NewCommand("rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunInDir(repoPath)
+			if err != nil {
+				log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
+				continue
+			}
+			parents := strings.Split(strings.TrimSpace(parentsString), " ")
+			if len(parents) < 3 {
+				continue
+			}
+
+			// we should recalculate
+			args := append([]string{"merge-base", "--"}, parents[1:]...)
+			args = append(args, gitRefName)
+
+			pr.MergeBase, err = git.NewCommand(args...).RunInDir(repoPath)
+			if err != nil {
+				log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
+				continue
+			}
+			pr.MergeBase = strings.TrimSpace(pr.MergeBase)
+			x.ID(pr.ID).Cols("merge_base").Update(pr)
+		}
+	}
+	return nil
+}
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 5fb5bfa2bb..c29cfb81b2 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -308,7 +308,6 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C
 
 	compareInfo, err := ctx.Repo.GitRepo.GetCompareInfo(ctx.Repo.Repository.RepoPath(),
 		pull.MergeBase, pull.GetGitRefName())
-
 	if err != nil {
 		if strings.Contains(err.Error(), "fatal: Not a valid object name") {
 			ctx.Data["IsPullRequestBroken"] = true
@@ -360,9 +359,40 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
 		ctx.Data["IsPullRequestBroken"] = true
 		ctx.Data["BaseTarget"] = pull.BaseBranch
 		ctx.Data["HeadTarget"] = pull.HeadBranch
-		ctx.Data["NumCommits"] = 0
-		ctx.Data["NumFiles"] = 0
-		return nil
+
+		sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
+			return nil
+		}
+		commitStatuses, err := models.GetLatestCommitStatus(repo, sha, 0)
+		if err != nil {
+			ctx.ServerError("GetLatestCommitStatus", err)
+			return nil
+		}
+		if len(commitStatuses) > 0 {
+			ctx.Data["LatestCommitStatuses"] = commitStatuses
+			ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(commitStatuses)
+		}
+
+		compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
+			pull.MergeBase, pull.GetGitRefName())
+		if err != nil {
+			if strings.Contains(err.Error(), "fatal: Not a valid object name") {
+				ctx.Data["IsPullRequestBroken"] = true
+				ctx.Data["BaseTarget"] = pull.BaseBranch
+				ctx.Data["NumCommits"] = 0
+				ctx.Data["NumFiles"] = 0
+				return nil
+			}
+
+			ctx.ServerError("GetCompareInfo", err)
+			return nil
+		}
+
+		ctx.Data["NumCommits"] = compareInfo.Commits.Len()
+		ctx.Data["NumFiles"] = compareInfo.NumFiles
+		return compareInfo
 	}
 
 	var headBranchExist bool