mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
fix(doctor): remove broken mergebase check (#12023)
Fixes https://codeberg.org/forgejo/forgejo/issues/6163 Fixes https://codeberg.org/forgejo/forgejo/issues/3343 The merge base doctor check & fix was broken and could introduce irreversible "fixes" to wrong merge bases for PRs using the `fast-forward` and `rebase-and-merge` strategies. The mergebase fix was originally introduced in a migration [0] to fix an existing issue [1] in the merge code in 2020. Later added as a doctor command without explanation [2]. We decided to remove this check, as there is no apparent reason for it to still be necessary or any PR merge base state being out of sync with the current implementation. It does more harm to keep the code in and there is no way to fix `fast-forward` and `rebase-and-merge` PRs, due to their merge implementation. `fast-forward`: The git state inherently cannot reconstruct a merge base in this scenario by design. `rebase-and-merge`: Is rebased on a temporary repository clone and thus might receive a different merge base, depending on how far the target branch is ahead. [0]:4a2b76d9c8[1]:4a2b76d9c8[2]:d26885e2bf (diff-84d6d60112991392d6ba2cae4cd919fb3ee8afb8)## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12023 Reviewed-by: Otto <otto@codeberg.org> Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Co-authored-by: Saibotk <git@saibotk.de> Co-committed-by: Saibotk <git@saibotk.de>
This commit is contained in:
Saibotk
Mathieu Fenniak
parent
bdd2a1def7
commit
8154ea5bea
1 changed files with 0 additions and 114 deletions
|
|
@ -1,114 +0,0 @@
|
|||
// Copyright 2020 The Gitea Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package doctor
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"strings"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
issues_model "forgejo.org/models/issues"
|
||||
repo_model "forgejo.org/models/repo"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/log"
|
||||
|
||||
"xorm.io/builder"
|
||||
)
|
||||
|
||||
func iteratePRs(ctx context.Context, repo *repo_model.Repository, each func(*repo_model.Repository, *issues_model.PullRequest) error) error {
|
||||
return db.Iterate(
|
||||
ctx,
|
||||
builder.Eq{"base_repo_id": repo.ID},
|
||||
func(ctx context.Context, bean *issues_model.PullRequest) error {
|
||||
return each(repo, bean)
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) error {
|
||||
numRepos := 0
|
||||
numPRs := 0
|
||||
numPRsUpdated := 0
|
||||
err := iterateRepositories(ctx, func(repo *repo_model.Repository) error {
|
||||
numRepos++
|
||||
return iteratePRs(ctx, repo, func(repo *repo_model.Repository, pr *issues_model.PullRequest) error {
|
||||
numPRs++
|
||||
pr.BaseRepo = repo
|
||||
repoPath := repo.RepoPath()
|
||||
|
||||
oldMergeBase := pr.MergeBase
|
||||
|
||||
if !pr.HasMerged {
|
||||
var err error
|
||||
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base").AddDashesAndList(pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
|
||||
if err != nil {
|
||||
var err2 error
|
||||
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
|
||||
if err2 != nil {
|
||||
logger.Warn("Unable to get merge base for PR ID %d, #%d onto %s in %s/%s. Error: %v & %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err, err2)
|
||||
return nil
|
||||
}
|
||||
}
|
||||
} else {
|
||||
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
|
||||
if err != nil {
|
||||
logger.Warn("Unable to get parents for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
|
||||
return nil
|
||||
}
|
||||
parents := strings.Split(strings.TrimSpace(parentsString), " ")
|
||||
if len(parents) < 2 {
|
||||
return nil
|
||||
}
|
||||
|
||||
refs := append([]string{}, parents[1:]...)
|
||||
refs = append(refs, pr.GetGitRefName())
|
||||
cmd := git.NewCommand(ctx, "merge-base").AddDashesAndList(refs...)
|
||||
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
|
||||
if err != nil {
|
||||
logger.Warn("Unable to get merge base for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
|
||||
return nil
|
||||
}
|
||||
}
|
||||
pr.MergeBase = strings.TrimSpace(pr.MergeBase)
|
||||
if pr.MergeBase != oldMergeBase {
|
||||
if autofix {
|
||||
if err := pr.UpdateCols(ctx, "merge_base"); err != nil {
|
||||
logger.Critical("Failed to update merge_base. ERROR: %v", err)
|
||||
return fmt.Errorf("Failed to update merge_base. ERROR: %w", err)
|
||||
}
|
||||
} else {
|
||||
logger.Info("#%d onto %s in %s/%s: MergeBase should be %s but is %s", pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, oldMergeBase, pr.MergeBase)
|
||||
}
|
||||
numPRsUpdated++
|
||||
}
|
||||
return nil
|
||||
})
|
||||
})
|
||||
|
||||
if autofix {
|
||||
logger.Info("%d PR mergebases updated of %d PRs total in %d repos", numPRsUpdated, numPRs, numRepos)
|
||||
} else {
|
||||
if numPRsUpdated == 0 {
|
||||
logger.Info("All %d PRs in %d repos have a correct mergebase", numPRs, numRepos)
|
||||
} else if err == nil {
|
||||
logger.Critical("%d PRs with incorrect mergebases of %d PRs total in %d repos", numPRsUpdated, numPRs, numRepos)
|
||||
return fmt.Errorf("%d PRs with incorrect mergebases of %d PRs total in %d repos", numPRsUpdated, numPRs, numRepos)
|
||||
} else {
|
||||
logger.Warn("%d PRs with incorrect mergebases of %d PRs total in %d repos", numPRsUpdated, numPRs, numRepos)
|
||||
}
|
||||
}
|
||||
|
||||
return err
|
||||
}
|
||||
|
||||
func init() {
|
||||
Register(&Check{
|
||||
Title: "Recalculate merge bases",
|
||||
Name: "recalculate-merge-bases",
|
||||
IsDefault: false,
|
||||
Run: checkPRMergeBase,
|
||||
Priority: 7,
|
||||
})
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue