third_party
/
gitea
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Improve trace logging for pulls and processes (#22633) (#22812)

Browse Source 
Backport #22633

Our trace logging is far from perfect and is difficult to follow.

This PR:

* Add trace logging for process manager add and remove.
* Fixes an errant read file for git refs in getMergeCommit
* Brings in the pullrequest `String` and `ColorFormat` methods
introduced in #22568
* Adds a lot more logging in to testPR etc.

Ref #22578

---------
Signed-off-by: Andrew Thornton <art27@cantab.net>
tokarchuk/v1.18
zeripath 2 years ago committed by GitHub
parent 656d5a144f
commit e4238583db
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 227 additions and 144 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 62
      models/issues/pull.go
  2. 7
      modules/log/colors.go
  3. 11
      modules/log/log.go
  4. 22
      modules/process/manager.go
  5. 2
      modules/process/manager_test.go
  6. 51
      routers/web/repo/pull.go
  7. 26
      services/automerge/automerge.go
  8. 138
      services/pull/check.go
  9. 2
      services/pull/patch.go

62
models/issues/pull.go
Unescape View File

@ -9,6 +9,7 @@ import (
"context" "context"
"fmt" "fmt"
"io" "io"
"strconv"
"strings" "strings"
"code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/db"
@ -133,6 +134,27 @@ const (
PullRequestStatusAncestor PullRequestStatusAncestor
) )
func (status PullRequestStatus) String() string {
switch status {
case PullRequestStatusConflict:
return "CONFLICT"
case PullRequestStatusChecking:
return "CHECKING"
case PullRequestStatusMergeable:
return "MERGEABLE"
case PullRequestStatusManuallyMerged:
return "MANUALLY_MERGED"
case PullRequestStatusError:
return "ERROR"
case PullRequestStatusEmpty:
return "EMPTY"
case PullRequestStatusAncestor:
return "ANCESTOR"
default:
return strconv.Itoa(int(status))
}
}
// PullRequestFlow the flow of pull request // PullRequestFlow the flow of pull request
type PullRequestFlow int type PullRequestFlow int
@ -204,6 +226,42 @@ func DeletePullsByBaseRepoID(ctx context.Context, repoID int64) error {
return err return err
} }
// ColorFormat writes a colored string to identify this struct
func (pr *PullRequest) ColorFormat(s fmt.State) {
if pr == nil {
log.ColorFprintf(s, "PR[%d]%s#%d[%s...%s:%s]",
log.NewColoredIDValue(0),
log.NewColoredValue("<nil>/<nil>"),
log.NewColoredIDValue(0),
log.NewColoredValue("<nil>"),
log.NewColoredValue("<nil>/<nil>"),
log.NewColoredValue("<nil>"),
)
return
}
log.ColorFprintf(s, "PR[%d]", log.NewColoredIDValue(pr.ID))
if pr.BaseRepo != nil {
log.ColorFprintf(s, "%s#%d[%s...", log.NewColoredValue(pr.BaseRepo.FullName()),
log.NewColoredIDValue(pr.Index), log.NewColoredValue(pr.BaseBranch))
} else {
log.ColorFprintf(s, "Repo[%d]#%d[%s...", log.NewColoredIDValue(pr.BaseRepoID),
log.NewColoredIDValue(pr.Index), log.NewColoredValue(pr.BaseBranch))
}
if pr.HeadRepoID == pr.BaseRepoID {
log.ColorFprintf(s, "%s]", log.NewColoredValue(pr.HeadBranch))
} else if pr.HeadRepo != nil {
log.ColorFprintf(s, "%s:%s]", log.NewColoredValue(pr.HeadRepo.FullName()), log.NewColoredValue(pr.HeadBranch))
} else {
log.ColorFprintf(s, "Repo[%d]:%s]", log.NewColoredIDValue(pr.HeadRepoID), log.NewColoredValue(pr.HeadBranch))
}
}
// String represents the pr as a simple string
func (pr *PullRequest) String() string {
return log.ColorFormatAsString(pr)
}
// MustHeadUserName returns the HeadRepo's username if failed return blank // MustHeadUserName returns the HeadRepo's username if failed return blank
func (pr *PullRequest) MustHeadUserName() string { func (pr *PullRequest) MustHeadUserName() string {
if err := pr.LoadHeadRepo(); err != nil { if err := pr.LoadHeadRepo(); err != nil {
@ -255,7 +313,7 @@ func (pr *PullRequest) LoadHeadRepoCtx(ctx context.Context) (err error) {
pr.HeadRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.HeadRepoID) pr.HeadRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.HeadRepoID)
if err != nil && !repo_model.IsErrRepoNotExist(err) { // Head repo maybe deleted, but it should still work if err != nil && !repo_model.IsErrRepoNotExist(err) { // Head repo maybe deleted, but it should still work
return fmt.Errorf("getRepositoryByID(head): %w", err) return fmt.Errorf("pr[%d].LoadHeadRepo[%d]: %w", pr.ID, pr.HeadRepoID, err)
} }
pr.isHeadRepoLoaded = true pr.isHeadRepoLoaded = true
} }
@ -290,7 +348,7 @@ func (pr *PullRequest) LoadBaseRepoCtx(ctx context.Context) (err error) {
pr.BaseRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.BaseRepoID) pr.BaseRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.BaseRepoID)
if err != nil { if err != nil {
return fmt.Errorf("repo_model.GetRepositoryByID(base): %w", err) return fmt.Errorf("pr[%d].LoadBaseRepo[%d]: %w", pr.ID, pr.BaseRepoID, err)
} }
return nil return nil
} }

7
modules/log/colors.go
Unescape View File

@ -384,6 +384,13 @@ func (cv *ColoredValue) Format(s fmt.State, c rune) {
s.Write(*cv.resetBytes) s.Write(*cv.resetBytes)
} }
// ColorFormatAsString returns the result of the ColorFormat without the color
func ColorFormatAsString(colorVal ColorFormatted) string {
s := new(strings.Builder)
_, _ = ColorFprintf(&protectedANSIWriter{w: s, mode: removeColor}, "%-v", colorVal)
return s.String()
}
// SetColorBytes will allow a user to set the colorBytes of a colored value // SetColorBytes will allow a user to set the colorBytes of a colored value
func (cv *ColoredValue) SetColorBytes(colorBytes []byte) { func (cv *ColoredValue) SetColorBytes(colorBytes []byte) {
cv.colorBytes = &colorBytes cv.colorBytes = &colorBytes

11
modules/log/log.go
Unescape View File

@ -10,6 +10,8 @@ import (
"runtime" "runtime"
"strings" "strings"
"sync" "sync"
"code.gitea.io/gitea/modules/process"
) )
type loggerMap struct { type loggerMap struct {
@ -286,6 +288,15 @@ func (l *LoggerAsWriter) Log(msg string) {
} }
func init() { func init() {
process.Trace = func(start bool, pid process.IDType, description string, parentPID process.IDType, typ string) {
if start && parentPID != "" {
Log(1, TRACE, "Start %s: %s (from %s) (%s)", NewColoredValue(pid, FgHiYellow), description, NewColoredValue(parentPID, FgYellow), NewColoredValue(typ, Reset))
} else if start {
Log(1, TRACE, "Start %s: %s (%s)", NewColoredValue(pid, FgHiYellow), description, NewColoredValue(typ, Reset))
} else {
Log(1, TRACE, "Done %s: %s", NewColoredValue(pid, FgHiYellow), NewColoredValue(description, Reset))
}
}
_, filename, _, _ := runtime.Caller(0) _, filename, _, _ := runtime.Caller(0)
prefix = strings.TrimSuffix(filename, "modules/log/log.go") prefix = strings.TrimSuffix(filename, "modules/log/log.go")
if prefix == filename { if prefix == filename {

22
modules/process/manager.go
Unescape View File

@ -7,6 +7,7 @@ package process
import ( import (
"context" "context"
"log"
"runtime/pprof" "runtime/pprof"
"strconv" "strconv"
"sync" "sync"
@ -44,6 +45,18 @@ type IDType string
// - it is simply an alias for context.CancelFunc and is only for documentary purposes // - it is simply an alias for context.CancelFunc and is only for documentary purposes
type FinishedFunc = context.CancelFunc type FinishedFunc = context.CancelFunc
var Trace = defaultTrace // this global can be overridden by particular logging packages - thus avoiding import cycles
func defaultTrace(start bool, pid IDType, description string, parentPID IDType, typ string) {
if start && parentPID != "" {
log.Printf("start process %s: %s (from %s) (%s)", pid, description, parentPID, typ)
} else if start {
log.Printf("start process %s: %s (%s)", pid, description, typ)
} else {
log.Printf("end process %s: %s", pid, description)
}
}
// Manager manages all processes and counts PIDs. // Manager manages all processes and counts PIDs.
type Manager struct { type Manager struct {
mutex sync.Mutex mutex sync.Mutex
@ -155,6 +168,7 @@ func (pm *Manager) Add(ctx context.Context, description string, cancel context.C
pm.processMap[pid] = process pm.processMap[pid] = process
pm.mutex.Unlock() pm.mutex.Unlock()
Trace(true, pid, description, parentPID, processType)
pprofCtx := pprof.WithLabels(ctx, pprof.Labels(DescriptionPProfLabel, description, PPIDPProfLabel, string(parentPID), PIDPProfLabel, string(pid), ProcessTypePProfLabel, processType)) pprofCtx := pprof.WithLabels(ctx, pprof.Labels(DescriptionPProfLabel, description, PPIDPProfLabel, string(parentPID), PIDPProfLabel, string(pid), ProcessTypePProfLabel, processType))
if currentlyRunning { if currentlyRunning {
@ -186,18 +200,12 @@ func (pm *Manager) nextPID() (start time.Time, pid IDType) {
return start, pid return start, pid
} }
// Remove a process from the ProcessManager.
func (pm *Manager) Remove(pid IDType) {
pm.mutex.Lock()
delete(pm.processMap, pid)
pm.mutex.Unlock()
}
func (pm *Manager) remove(process *process) { func (pm *Manager) remove(process *process) {
pm.mutex.Lock() pm.mutex.Lock()
defer pm.mutex.Unlock() defer pm.mutex.Unlock()
if p := pm.processMap[process.PID]; p == process { if p := pm.processMap[process.PID]; p == process {
delete(pm.processMap, process.PID) delete(pm.processMap, process.PID)
Trace(false, process.PID, process.Description, process.ParentPID, process.Type)
} }
} }

2
modules/process/manager_test.go
Unescape View File

@ -83,7 +83,7 @@ func TestManager_Remove(t *testing.T) {
assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids got %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID()) assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids got %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID())
pm.Remove(GetPID(p2Ctx)) finished()
_, exists := pm.processMap[GetPID(p2Ctx)] _, exists := pm.processMap[GetPID(p2Ctx)]
assert.False(t, exists, "PID %d is in the list but shouldn't", GetPID(p2Ctx)) assert.False(t, exists, "PID %d is in the list but shouldn't", GetPID(p2Ctx))

51
routers/web/repo/pull.go
Unescape View File

@ -912,60 +912,55 @@ func MergePullRequest(ctx *context.Context) {
pr := issue.PullRequest pr := issue.PullRequest
pr.Issue = issue pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository pr.Issue.Repo = ctx.Repo.Repository
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged manualMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
forceMerge := form.ForceMerge != nil && *form.ForceMerge forceMerge := form.ForceMerge != nil && *form.ForceMerge
// start with merging by checking // start with merging by checking
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, forceMerge); err != nil { if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manualMerge, forceMerge); err != nil {
if errors.Is(err, pull_service.ErrIsClosed) { switch {
case errors.Is(err, pull_service.ErrIsClosed):
if issue.IsPull { if issue.IsPull {
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed")) ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed"))
ctx.Redirect(issue.Link())
} else { } else {
ctx.Flash.Error(ctx.Tr("repo.issues.closed_title")) ctx.Flash.Error(ctx.Tr("repo.issues.closed_title"))
ctx.Redirect(issue.Link())
} }
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { case errors.Is(err, pull_service.ErrUserNotAllowedToMerge):
ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed")) ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
ctx.Redirect(issue.Link()) case errors.Is(err, pull_service.ErrHasMerged):
} else if errors.Is(err, pull_service.ErrHasMerged) {
ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged")) ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged"))
ctx.Redirect(issue.Link()) case errors.Is(err, pull_service.ErrIsWorkInProgress):
} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip")) ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
ctx.Redirect(issue.Link()) case errors.Is(err, pull_service.ErrNotMergableState):
} else if errors.Is(err, pull_service.ErrNotMergableState) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link()) case models.IsErrDisallowedToMerge(err):
} else if models.IsErrDisallowedToMerge(err) {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
ctx.Redirect(issue.Link()) case asymkey_service.IsErrWontSign(err):
} else if asymkey_service.IsErrWontSign(err) { ctx.Flash.Error(err.Error()) // has no translation ...
ctx.Flash.Error(err.Error()) // has not translation ... case errors.Is(err, pull_service.ErrDependenciesLeft):
ctx.Redirect(issue.Link())
} else if errors.Is(err, pull_service.ErrDependenciesLeft) {
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked")) ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
ctx.Redirect(issue.Link()) default:
} else {
ctx.ServerError("WebCheck", err) ctx.ServerError("WebCheck", err)
return
} }
ctx.Redirect(issue.Link())
return return
} }
// handle manually-merged mark // handle manually-merged mark
if manuallMerge { if manualMerge {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if models.IsErrInvalidMergeStyle(err) { switch {
case models.IsErrInvalidMergeStyle(err):
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
ctx.Redirect(issue.Link()) case strings.Contains(err.Error(), "Wrong commit ID"):
} else if strings.Contains(err.Error(), "Wrong commit ID") {
ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id")) ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id"))
ctx.Redirect(issue.Link()) default:
} else {
ctx.ServerError("MergedManually", err) ctx.ServerError("MergedManually", err)
}
return return
} }
}
ctx.Redirect(issue.Link()) ctx.Redirect(issue.Link())
return return

26
services/automerge/automerge.go
Unescape View File

@ -165,7 +165,7 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.
func handlePull(pullID int64, sha string) { func handlePull(pullID int64, sha string) {
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(),
fmt.Sprintf("Handle AutoMerge of pull[%d] with sha[%s]", pullID, sha)) fmt.Sprintf("Handle AutoMerge of PR[%d] with sha[%s]", pullID, sha))
defer finished() defer finished()
pr, err := issues_model.GetPullRequestByID(ctx, pullID) pr, err := issues_model.GetPullRequestByID(ctx, pullID)
@ -177,7 +177,7 @@ func handlePull(pullID int64, sha string) {
// Check if there is a scheduled pr in the db // Check if there is a scheduled pr in the db
exists, scheduledPRM, err := pull_model.GetScheduledMergeByPullID(ctx, pr.ID) exists, scheduledPRM, err := pull_model.GetScheduledMergeByPullID(ctx, pr.ID)
if err != nil { if err != nil {
log.Error("pull[%d] GetScheduledMergeByPullID: %v", pr.ID, err) log.Error("%-v GetScheduledMergeByPullID: %v", pr, err)
return return
} }
if !exists { if !exists {
@ -189,13 +189,13 @@ func handlePull(pullID int64, sha string) {
// did not succeed or was not finished yet. // did not succeed or was not finished yet.
if err = pr.LoadHeadRepoCtx(ctx); err != nil { if err = pr.LoadHeadRepoCtx(ctx); err != nil {
log.Error("pull[%d] LoadHeadRepoCtx: %v", pr.ID, err) log.Error("%-v LoadHeadRepo: %v", pr, err)
return return
} }
headGitRepo, err := git.OpenRepository(ctx, pr.HeadRepo.RepoPath()) headGitRepo, err := git.OpenRepository(ctx, pr.HeadRepo.RepoPath())
if err != nil { if err != nil {
log.Error("OpenRepository: %v", err) log.Error("OpenRepository %-v: %v", pr.HeadRepo, err)
return return
} }
defer headGitRepo.Close() defer headGitRepo.Close()
@ -203,40 +203,40 @@ func handlePull(pullID int64, sha string) {
headBranchExist := headGitRepo.IsBranchExist(pr.HeadBranch) headBranchExist := headGitRepo.IsBranchExist(pr.HeadBranch)
if pr.HeadRepo == nil || !headBranchExist { if pr.HeadRepo == nil || !headBranchExist {
log.Warn("Head branch of auto merge pr does not exist [HeadRepoID: %d, Branch: %s, PR ID: %d]", pr.HeadRepoID, pr.HeadBranch, pr.ID) log.Warn("Head branch of auto merge %-v does not exist [HeadRepoID: %d, Branch: %s]", pr, pr.HeadRepoID, pr.HeadBranch)
return return
} }
// Check if all checks succeeded // Check if all checks succeeded
pass, err := pull_service.IsPullCommitStatusPass(ctx, pr) pass, err := pull_service.IsPullCommitStatusPass(ctx, pr)
if err != nil { if err != nil {
log.Error("IsPullCommitStatusPass: %v", err) log.Error("%-v IsPullCommitStatusPass: %v", pr, err)
return return
} }
if !pass { if !pass {
log.Info("Scheduled auto merge pr has unsuccessful status checks [PullID: %d]", pr.ID) log.Info("Scheduled auto merge %-v has unsuccessful status checks", pr)
return return
} }
// Merge if all checks succeeded // Merge if all checks succeeded
doer, err := user_model.GetUserByIDCtx(ctx, scheduledPRM.DoerID) doer, err := user_model.GetUserByIDCtx(ctx, scheduledPRM.DoerID)
if err != nil { if err != nil {
log.Error("GetUserByIDCtx: %v", err) log.Error("Unable to get scheduled User[%d]: %v", scheduledPRM.DoerID, err)
return return
} }
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer) perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer)
if err != nil { if err != nil {
log.Error("GetUserRepoPermission: %v", err) log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err)
return return
} }
if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, false, false); err != nil { if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, false, false); err != nil {
if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) { if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) {
log.Info("PR %d was scheduled to automerge by an unauthorized user", pr.ID) log.Info("%-v was scheduled to automerge by an unauthorized user", pr)
return return
} }
log.Error("pull[%d] CheckPullMergable: %v", pr.ID, err) log.Error("%-v CheckPullMergable: %v", pr, err)
return return
} }
@ -245,13 +245,13 @@ func handlePull(pullID int64, sha string) {
baseGitRepo = headGitRepo baseGitRepo = headGitRepo
} else { } else {
if err = pr.LoadBaseRepoCtx(ctx); err != nil { if err = pr.LoadBaseRepoCtx(ctx); err != nil {
log.Error("LoadBaseRepoCtx: %v", err) log.Error("%-v LoadBaseRepo: %v", pr, err)
return return
} }
baseGitRepo, err = git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) baseGitRepo, err = git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
if err != nil { if err != nil {
log.Error("OpenRepository: %v", err) log.Error("OpenRepository %-v: %v", pr.BaseRepo, err)
return return
} }
defer baseGitRepo.Close() defer baseGitRepo.Close()

138
services/pull/check.go
Unescape View File

@ -9,7 +9,6 @@ import (
"context" "context"
"errors" "errors"
"fmt" "fmt"
"os"
"strconv" "strconv"
"strings" "strings"
@ -27,7 +26,6 @@ import (
"code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/process"
"code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/queue"
"code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
asymkey_service "code.gitea.io/gitea/services/asymkey" asymkey_service "code.gitea.io/gitea/services/asymkey"
) )
@ -50,14 +48,14 @@ func AddToTaskQueue(pr *issues_model.PullRequest) {
pr.Status = issues_model.PullRequestStatusChecking pr.Status = issues_model.PullRequestStatusChecking
err := pr.UpdateColsIfNotMerged("status") err := pr.UpdateColsIfNotMerged("status")
if err != nil { if err != nil {
log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) log.Error("AddToTaskQueue(%-v).UpdateCols.(add to queue): %v", pr, err)
} else { } else {
log.Trace("Adding PR ID: %d to the test pull requests queue", pr.ID) log.Trace("Adding %-v to the test pull requests queue", pr)
} }
return err return err
}) })
if err != nil && err != queue.ErrAlreadyInQueue { if err != nil && err != queue.ErrAlreadyInQueue {
log.Error("Error adding prID %d to the test pull requests queue: %v", pr.ID, err) log.Error("Error adding %-v to the test pull requests queue: %v", pr, err)
} }
} }
@ -69,12 +67,14 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
} }
if err := pr.LoadIssueCtx(ctx); err != nil { if err := pr.LoadIssueCtx(ctx); err != nil {
log.Error("Unable to load issue[%d] for %-v: %v", pr.IssueID, pr, err)
return err return err
} else if pr.Issue.IsClosed { } else if pr.Issue.IsClosed {
return ErrIsClosed return ErrIsClosed
} }
if allowedMerge, err := IsUserAllowedToMerge(ctx, pr, *perm, doer); err != nil { if allowedMerge, err := IsUserAllowedToMerge(ctx, pr, *perm, doer); err != nil {
log.Error("Error whilst checking if %-v is allowed to merge %-v: %v", doer, pr, err)
return err return err
} else if !allowedMerge { } else if !allowedMerge {
return ErrUserNotAllowedToMerge return ErrUserNotAllowedToMerge
@ -98,15 +98,19 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
} }
if err := CheckPullBranchProtections(ctx, pr, false); err != nil { if err := CheckPullBranchProtections(ctx, pr, false); err != nil {
if models.IsErrDisallowedToMerge(err) { if !models.IsErrDisallowedToMerge(err) {
if force { log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err)
if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil {
return err2
} else if !isRepoAdmin {
return err return err
} }
if !force {
return err
} }
} else {
if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil {
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, err2)
return err2
} else if !isRepoAdmin {
return err return err
} }
} }
@ -143,7 +147,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer
// checkAndUpdateStatus checks if pull request is possible to leaving checking status, // checkAndUpdateStatus checks if pull request is possible to leaving checking status,
// and set to be either conflict or mergeable. // and set to be either conflict or mergeable.
func checkAndUpdateStatus(pr *issues_model.PullRequest) { func checkAndUpdateStatus(pr *issues_model.PullRequest) {
// Status is not changed to conflict means mergeable. // If status has not been changed to conflict by testPatch then we are mergeable
if pr.Status == issues_model.PullRequestStatusChecking { if pr.Status == issues_model.PullRequestStatusChecking {
pr.Status = issues_model.PullRequestStatusMergeable pr.Status = issues_model.PullRequestStatusMergeable
} }
@ -151,79 +155,69 @@ func checkAndUpdateStatus(pr *issues_model.PullRequest) {
// Make sure there is no waiting test to process before leaving the checking status. // Make sure there is no waiting test to process before leaving the checking status.
has, err := prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10)) has, err := prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10))
if err != nil { if err != nil {
log.Error("Unable to check if the queue is waiting to reprocess pr.ID %d. Error: %v", pr.ID, err) log.Error("Unable to check if the queue is waiting to reprocess %-v. Error: %v", pr, err)
} }
if !has { if has {
if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Trace("Not updating status for %-v as it is due to be rechecked", pr)
log.Error("Update[%d]: %v", pr.ID, err) return
} }
if err := pr.UpdateColsIfNotMerged("merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil {
log.Error("Update[%-v]: %v", pr, err)
} }
} }
// getMergeCommit checks if a pull request got merged // getMergeCommit checks if a pull request has been merged
// Returns the git.Commit of the pull request if merged // Returns the git.Commit of the pull request if merged
func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Commit, error) { func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Commit, error) {
if pr.BaseRepo == nil { if err := pr.LoadBaseRepoCtx(ctx); err != nil {
var err error return nil, fmt.Errorf("unable to load base repo for %s: %w", pr, err)
pr.BaseRepo, err = repo_model.GetRepositoryByID(pr.BaseRepoID)
if err != nil {
return nil, fmt.Errorf("GetRepositoryByID: %w", err)
}
}
indexTmpPath, err := os.MkdirTemp(os.TempDir(), "gitea-"+pr.BaseRepo.Name)
if err != nil {
return nil, fmt.Errorf("Failed to create temp dir for repository %s: %w", pr.BaseRepo.RepoPath(), err)
}
defer func() {
if err := util.RemoveAll(indexTmpPath); err != nil {
log.Warn("Unable to remove temporary index path: %s: Error: %v", indexTmpPath, err)
} }
}()
headFile := pr.GetGitRefName() prHeadRef := pr.GetGitRefName()
// Check if a pull request is merged into BaseBranch // Check if the pull request is merged into BaseBranch
_, _, err = git.NewCommand(ctx, "merge-base", "--is-ancestor").AddDynamicArguments(headFile, pr.BaseBranch). if _, _, err := git.NewCommand(ctx, "merge-base", "--is-ancestor").
RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath(), Env: []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}}) AddDynamicArguments(prHeadRef, pr.BaseBranch).
if err != nil { RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}); err != nil {
// Errors are signaled by a non-zero status that is not 1
if strings.Contains(err.Error(), "exit status 1") { if strings.Contains(err.Error(), "exit status 1") {
// prHeadRef is not an ancestor of the base branch
return nil, nil return nil, nil
} }
return nil, fmt.Errorf("git merge-base --is-ancestor: %w", err) // Errors are signaled by a non-zero status that is not 1
return nil, fmt.Errorf("%-v git merge-base --is-ancestor: %w", pr, err)
} }
commitIDBytes, err := os.ReadFile(pr.BaseRepo.RepoPath() + "/" + headFile) // If merge-base successfully exits then prHeadRef is an ancestor of pr.BaseBranch
// Find the head commit id
prHeadCommitID, err := git.GetFullCommitID(ctx, pr.BaseRepo.RepoPath(), prHeadRef)
if err != nil { if err != nil {
return nil, fmt.Errorf("ReadFile(%s): %w", headFile, err) return nil, fmt.Errorf("GetFullCommitID(%s) in %s: %w", prHeadRef, pr.BaseRepo.FullName(), err)
}
commitID := string(commitIDBytes)
if len(commitID) < git.SHAFullLength {
return nil, fmt.Errorf(`ReadFile(%s): invalid commit-ID "%s"`, headFile, commitID)
} }
cmd := commitID[:git.SHAFullLength] + ".." + pr.BaseBranch
// Get the commit from BaseBranch where the pull request got merged // Get the commit from BaseBranch where the pull request got merged
mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse").AddDynamicArguments(cmd). mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse").
RunStdString(&git.RunOpts{Dir: "", Env: []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}}) AddDynamicArguments(prHeadCommitID + ".." + pr.BaseBranch).
RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()})
if err != nil { if err != nil {
return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err) return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err)
} else if len(mergeCommit) < git.SHAFullLength { } else if len(mergeCommit) < git.SHAFullLength {
// PR was maybe fast-forwarded, so just use last commit of PR // PR was maybe fast-forwarded, so just use last commit of PR
mergeCommit = commitID[:git.SHAFullLength] mergeCommit = prHeadCommitID
} }
mergeCommit = strings.TrimSpace(mergeCommit)
gitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) gitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
if err != nil { if err != nil {
return nil, fmt.Errorf("OpenRepository: %w", err) return nil, fmt.Errorf("%-v OpenRepository: %w", pr.BaseRepo, err)
} }
defer gitRepo.Close() defer gitRepo.Close()
commit, err := gitRepo.GetCommit(mergeCommit[:git.SHAFullLength]) commit, err := gitRepo.GetCommit(mergeCommit)
if err != nil { if err != nil {
return nil, fmt.Errorf("GetMergeCommit[%v]: %w", mergeCommit[:git.SHAFullLength], err) return nil, fmt.Errorf("GetMergeCommit[%s]: %w", mergeCommit, err)
} }
return commit, nil return commit, nil
@ -233,7 +227,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
// When a pull request got manually merged mark the pull request as merged // When a pull request got manually merged mark the pull request as merged
func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
if err := pr.LoadBaseRepoCtx(ctx); err != nil { if err := pr.LoadBaseRepoCtx(ctx); err != nil {
log.Error("PullRequest[%d].LoadBaseRepo: %v", pr.ID, err) log.Error("%-v LoadBaseRepo: %v", pr, err)
return false return false
} }
@ -243,16 +237,21 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
return false return false
} }
} else { } else {
log.Error("PullRequest[%d].BaseRepo.GetUnit(unit.TypePullRequests): %v", pr.ID, err) log.Error("%-v BaseRepo.GetUnit(unit.TypePullRequests): %v", pr, err)
return false return false
} }
commit, err := getMergeCommit(ctx, pr) commit, err := getMergeCommit(ctx, pr)
if err != nil { if err != nil {
log.Error("PullRequest[%d].getMergeCommit: %v", pr.ID, err) log.Error("%-v getMergeCommit: %v", pr, err)
return false return false
} }
if commit != nil {
if commit == nil {
// no merge commit found
return false
}
pr.MergedCommitID = commit.ID.String() pr.MergedCommitID = commit.ID.String()
pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
pr.Status = issues_model.PullRequestStatusManuallyMerged pr.Status = issues_model.PullRequestStatusManuallyMerged
@ -262,7 +261,7 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
if merger == nil { if merger == nil {
if pr.BaseRepo.Owner == nil { if pr.BaseRepo.Owner == nil {
if err = pr.BaseRepo.GetOwner(ctx); err != nil { if err = pr.BaseRepo.GetOwner(ctx); err != nil {
log.Error("BaseRepo.GetOwner[%d]: %v", pr.ID, err) log.Error("%-v BaseRepo.GetOwner: %v", pr, err)
return false return false
} }
} }
@ -272,7 +271,7 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
pr.MergerID = merger.ID pr.MergerID = merger.ID
if merged, err := pr.SetMerged(ctx); err != nil { if merged, err := pr.SetMerged(ctx); err != nil {
log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) log.Error("%-v setMerged : %v", pr, err)
return false return false
} else if !merged { } else if !merged {
return false return false
@ -280,10 +279,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
notification.NotifyMergePullRequest(pr, merger) notification.NotifyMergePullRequest(pr, merger)
log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) log.Info("manuallyMerged[%-v]: Marked as manually merged into %s/%s by commit id: %s", pr, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
return true return true
}
return false
} }
// InitializePullRequests checks and tests untested patches of pull requests. // InitializePullRequests checks and tests untested patches of pull requests.
@ -299,10 +296,10 @@ func InitializePullRequests(ctx context.Context) {
return return
default: default:
if err := prPatchCheckerQueue.PushFunc(strconv.FormatInt(prID, 10), func() error { if err := prPatchCheckerQueue.PushFunc(strconv.FormatInt(prID, 10), func() error {
log.Trace("Adding PR ID: %d to the pull requests patch checking queue", prID) log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID)
return nil return nil
}); err != nil { }); err != nil {
log.Error("Error adding prID: %s to the pull requests patch checking queue %v", prID, err) log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err)
} }
} }
} }
@ -326,23 +323,30 @@ func testPR(id int64) {
pr, err := issues_model.GetPullRequestByID(ctx, id) pr, err := issues_model.GetPullRequestByID(ctx, id)
if err != nil { if err != nil {
log.Error("GetPullRequestByID[%d]: %v", id, err) log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err)
return return
} }
log.Trace("Testing %-v", pr)
defer func() {
log.Trace("Done testing %-v (status: %s)", pr, pr.Status)
}()
if pr.HasMerged { if pr.HasMerged {
log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID)
return return
} }
if manuallyMerged(ctx, pr) { if manuallyMerged(ctx, pr) {
log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID)
return return
} }
if err := TestPatch(pr); err != nil { if err := TestPatch(pr); err != nil {
log.Error("testPatch[%d]: %v", pr.ID, err) log.Error("testPatch[%-v]: %v", pr, err)
pr.Status = issues_model.PullRequestStatusError pr.Status = issues_model.PullRequestStatusError
if err := pr.UpdateCols("status"); err != nil { if err := pr.UpdateCols("status"); err != nil {
log.Error("update pr [%d] status to PullRequestStatusError failed: %v", pr.ID, err) log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err)
} }
return return
} }

2
services/pull/patch.go
Unescape View File

@ -60,7 +60,7 @@ var patchErrorSuffices = []string{
// TestPatch will test whether a simple patch will apply // TestPatch will test whether a simple patch will apply
func TestPatch(pr *issues_model.PullRequest) error { func TestPatch(pr *issues_model.PullRequest) error {
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: Repo[%d]#%d", pr.BaseRepoID, pr.Index)) ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr))
defer finished() defer finished()
// Clone base repo. // Clone base repo.

Write Preview
Loading…
Cancel
Save