From 648eb3ff4f776d8a48aeea505b26b7f1a6371cae Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 14 Sep 2020 21:15:34 +0100 Subject: [PATCH 1/3] Stop cloning unnecessarily on PR update Fix #12740 Signed-off-by: Andrew Thornton --- services/pull/pull.go | 70 +++++++++++-------------------------------- 1 file changed, 17 insertions(+), 53 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 790b747aa74b0..4849a9ec653c2 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -215,18 +215,6 @@ func checkForInvalidation(requests models.PullRequestList, repoID int64, doer *m return nil } -func addHeadRepoTasks(prs []*models.PullRequest) { - for _, pr := range prs { - log.Trace("addHeadRepoTasks[%d]: composing new test task", pr.ID) - if err := PushToBaseRepo(pr); err != nil { - log.Error("PushToBaseRepo: %v", err) - continue - } - - AddToTaskQueue(pr) - } -} - // AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch, // and generate new patch for testing as needed. func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) { @@ -283,8 +271,14 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy } } - addHeadRepoTasks(prs) for _, pr := range prs { + log.Trace("Updating PR[%d]: composing new test task", pr.ID) + if err := PushToBaseRepo(pr); err != nil { + log.Error("PushToBaseRepo: %v", err) + continue + } + + AddToTaskQueue(pr) comment, err := models.CreatePushPullComment(doer, pr, oldCommitID, newCommitID) if err == nil && comment != nil { notification.NotifyPullRequestPushCommits(doer, pr, comment) @@ -389,46 +383,23 @@ func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID st func PushToBaseRepo(pr *models.PullRequest) (err error) { log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitRefName()) - // Clone base repo. - tmpBasePath, err := models.CreateTemporaryPath("pull") - if err != nil { - log.Error("CreateTemporaryPath: %v", err) - return err - } - defer func() { - err := models.RemoveTemporaryPath(tmpBasePath) - if err != nil { - log.Error("Error whilst removing temporary path: %s Error: %v", tmpBasePath, err) - } - }() - if err := pr.LoadHeadRepo(); err != nil { log.Error("Unable to load head repository for PR[%d] Error: %v", pr.ID, err) return err } headRepoPath := pr.HeadRepo.RepoPath() - if err := git.Clone(headRepoPath, tmpBasePath, git.CloneRepoOptions{ - Bare: true, - Shared: true, - Branch: pr.HeadBranch, - Quiet: true, - }); err != nil { - log.Error("git clone tmpBasePath: %v", err) - return err - } - gitRepo, err := git.OpenRepository(tmpBasePath) - if err != nil { - return fmt.Errorf("OpenRepository: %v", err) - } - defer gitRepo.Close() - if err := pr.LoadBaseRepo(); err != nil { log.Error("Unable to load base repository for PR[%d] Error: %v", pr.ID, err) return err } - if err := gitRepo.AddRemote("base", pr.BaseRepo.RepoPath(), false); err != nil { - return fmt.Errorf("tmpGitRepo.AddRemote: %v", err) + baseRepoPath := pr.BaseRepo.RepoPath() + + if err = pr.LoadIssue(); err != nil { + return fmt.Errorf("unable to load issue %d for pr %d: %v", pr.IssueID, pr.ID, err) + } + if err = pr.Issue.LoadPoster(); err != nil { + return fmt.Errorf("unable to load poster %d for pr %d: %v", pr.Issue.PosterID, pr.ID, err) } headFile := pr.GetGitRefName() @@ -438,16 +409,9 @@ func PushToBaseRepo(pr *models.PullRequest) (err error) { _ = util.Remove(file) - if err = pr.LoadIssue(); err != nil { - return fmt.Errorf("unable to load issue %d for pr %d: %v", pr.IssueID, pr.ID, err) - } - if err = pr.Issue.LoadPoster(); err != nil { - return fmt.Errorf("unable to load poster %d for pr %d: %v", pr.Issue.PosterID, pr.ID, err) - } - - if err = git.Push(tmpBasePath, git.PushOptions{ - Remote: "base", - Branch: fmt.Sprintf("%s:%s", pr.HeadBranch, headFile), + if err := git.Push(headRepoPath, git.PushOptions{ + Remote: baseRepoPath, + Branch: pr.HeadBranch + ":" + headFile, Force: true, // Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/... Env: models.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo), From 47c1dcbae4c8c215736ec93b77a40de49edf6385 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 14 Sep 2020 21:20:47 +0100 Subject: [PATCH 2/3] Remove unnecessary delete Signed-off-by: Andrew Thornton --- services/pull/pull.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 4849a9ec653c2..c4f03e99c96f8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -10,7 +10,6 @@ import ( "context" "encoding/json" "fmt" - "path/filepath" "strings" "time" @@ -20,7 +19,6 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" issue_service "code.gitea.io/gitea/services/issue" "github.com/unknwon/com" @@ -402,16 +400,9 @@ func PushToBaseRepo(pr *models.PullRequest) (err error) { return fmt.Errorf("unable to load poster %d for pr %d: %v", pr.Issue.PosterID, pr.ID, err) } - headFile := pr.GetGitRefName() - - // Remove head in case there is a conflict. - file := filepath.Join(pr.BaseRepo.RepoPath(), headFile) - - _ = util.Remove(file) - if err := git.Push(headRepoPath, git.PushOptions{ Remote: baseRepoPath, - Branch: pr.HeadBranch + ":" + headFile, + Branch: pr.HeadBranch + ":" + pr.GetGitRefName(), Force: true, // Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/... Env: models.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo), From 5505cc0e879c10942e60bc4347247c6ad2210711 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 14 Sep 2020 21:56:10 +0100 Subject: [PATCH 3/3] fix lint Signed-off-by: Andrew Thornton --- services/pull/pull.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c4f03e99c96f8..53de7428546f8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -400,24 +400,26 @@ func PushToBaseRepo(pr *models.PullRequest) (err error) { return fmt.Errorf("unable to load poster %d for pr %d: %v", pr.Issue.PosterID, pr.ID, err) } + gitRefName := pr.GetGitRefName() + if err := git.Push(headRepoPath, git.PushOptions{ Remote: baseRepoPath, - Branch: pr.HeadBranch + ":" + pr.GetGitRefName(), + Branch: pr.HeadBranch + ":" + gitRefName, Force: true, // Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/... Env: models.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo), }); err != nil { if git.IsErrPushOutOfDate(err) { // This should not happen as we're using force! - log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, err) + log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err) return err } else if git.IsErrPushRejected(err) { rejectErr := err.(*git.ErrPushRejected) - log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err) + log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err) return err } - log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, err) - return fmt.Errorf("Push: %s:%s %s:%s %v", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), headFile, err) + log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err) + return fmt.Errorf("Push: %s:%s %s:%s %v", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), gitRefName, err) } return nil