From 2c25e75dca62850ddc328ed46c7b7d27b61be90b Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 31 Mar 2020 14:42:44 +0100 Subject: [PATCH] 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 --- models/migrations/migrations.go | 2 + models/migrations/v128.go | 20 +++++-- models/migrations/v134.go | 96 +++++++++++++++++++++++++++++++++ routers/repo/pull.go | 38 +++++++++++-- 4 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 models/migrations/v134.go 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 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 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: %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