From 74179d1b5e739b3fa0d0915bb35d6b7596fd13af Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 13 Dec 2019 22:21:06 +0000 Subject: [PATCH] Remove SavePatch and generate patches on the fly (#9302) * Save patches to temporary files * Remove SavePatch and generate patches on the fly * Use ioutil.TempDir * fixup! Use ioutil.TempDir * fixup! fixup! Use ioutil.TempDir * RemoveAll LocalCopyPath() in initIntergrationTest * Default to status checking on PR creation * Remove unnecessary set to StatusChecking * Protect against unable to load repo * Handle conflicts * Restore original conflict setting * In TestPullRequests update status to StatusChecking before running TestPatch --- integrations/integration_test.go | 2 +- models/helper_directory.go | 18 +-- models/issue_xref_test.go | 4 +- models/pull.go | 187 +------------------------- models/pull_test.go | 2 - models/repo.go | 36 ------ modules/git/repo_compare.go | 32 ++--- modules/git/repo_compare_test.go | 4 +- routers/api/v1/repo/pull.go | 8 +- routers/repo/issue.go | 5 - routers/repo/pull.go | 71 ++-------- services/pull/check.go | 10 +- services/pull/merge.go | 78 +---------- services/pull/patch.go | 216 +++++++++++++++++++++++++++++++ services/pull/pull.go | 13 +- services/pull/temp_repo.go | 152 ++++++++++++++++++++++ 16 files changed, 432 insertions(+), 406 deletions(-) create mode 100644 services/pull/patch.go create mode 100644 services/pull/temp_repo.go diff --git a/integrations/integration_test.go b/integrations/integration_test.go index 4177493930..5da9e04c78 100644 --- a/integrations/integration_test.go +++ b/integrations/integration_test.go @@ -125,6 +125,7 @@ func initIntegrationTest() { setting.SetCustomPathAndConf("", "", "") setting.NewContext() + os.RemoveAll(models.LocalCopyPath()) setting.CheckLFSVersion() setting.InitDBConfig() @@ -182,7 +183,6 @@ func prepareTestEnv(t testing.TB, skip ...int) func() { deferFn := PrintCurrentTest(t, ourSkip) assert.NoError(t, models.LoadFixtures()) assert.NoError(t, os.RemoveAll(setting.RepoRootPath)) - assert.NoError(t, os.RemoveAll(models.LocalCopyPath())) assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) diff --git a/models/helper_directory.go b/models/helper_directory.go index 813f0577bc..8119ec91a3 100644 --- a/models/helper_directory.go +++ b/models/helper_directory.go @@ -6,15 +6,13 @@ package models import ( "fmt" + "io/ioutil" "os" "path" "path/filepath" - "time" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - - "github.com/unknwon/com" ) // LocalCopyPath returns the local repository temporary copy path. @@ -27,11 +25,15 @@ func LocalCopyPath() string { // CreateTemporaryPath creates a temporary path func CreateTemporaryPath(prefix string) (string, error) { - timeStr := com.ToStr(time.Now().Nanosecond()) // SHOULD USE SOMETHING UNIQUE - basePath := path.Join(LocalCopyPath(), prefix+"-"+timeStr+".git") - if err := os.MkdirAll(filepath.Dir(basePath), os.ModePerm); err != nil { - log.Error("Unable to create temporary directory: %s (%v)", basePath, err) - return "", fmt.Errorf("Failed to create dir %s: %v", basePath, err) + if err := os.MkdirAll(LocalCopyPath(), os.ModePerm); err != nil { + log.Error("Unable to create localcopypath directory: %s (%v)", LocalCopyPath(), err) + return "", fmt.Errorf("Failed to create localcopypath directory %s: %v", LocalCopyPath(), err) + } + basePath, err := ioutil.TempDir(LocalCopyPath(), prefix+".git") + if err != nil { + log.Error("Unable to create temporary directory: %s-*.git (%v)", prefix, err) + return "", fmt.Errorf("Failed to create dir %s-*.git: %v", prefix, err) + } return basePath, nil } diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index c13577e905..d8defd99c6 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -142,8 +142,8 @@ func testCreatePR(t *testing.T, repo, doer int64, title, content string) *PullRe r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository) d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User) i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: true} - pr := &PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base"} - assert.NoError(t, NewPullRequest(r, i, nil, nil, pr, nil)) + pr := &PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base", Status: PullRequestStatusMergeable} + assert.NoError(t, NewPullRequest(r, i, nil, nil, pr)) pr.Issue = i return pr } diff --git a/models/pull.go b/models/pull.go index 2bd79202f0..33adc3214f 100644 --- a/models/pull.go +++ b/models/pull.go @@ -6,22 +6,16 @@ package models import ( - "bufio" "fmt" "os" "path" - "path/filepath" - "strconv" "strings" - "time" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" - - "github.com/unknwon/com" ) // PullRequestType defines pull request type @@ -481,125 +475,12 @@ func (pr *PullRequest) SetMerged() (err error) { return nil } -// patchConflicts is a list of conflict description from Git. -var patchConflicts = []string{ - "patch does not apply", - "already exists in working directory", - "unrecognized input", - "error:", -} - -// TestPatch checks if patch can be merged to base repository without conflict. -func (pr *PullRequest) TestPatch() error { - return pr.testPatch(x) -} - -// testPatch checks if patch can be merged to base repository without conflict. -func (pr *PullRequest) testPatch(e Engine) (err error) { - if pr.BaseRepo == nil { - pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) - if err != nil { - return fmt.Errorf("GetRepositoryByID: %v", err) - } - } - - patchPath, err := pr.BaseRepo.patchPath(e, pr.Index) - if err != nil { - return fmt.Errorf("BaseRepo.PatchPath: %v", err) - } - - // Fast fail if patch does not exist, this assumes data is corrupted. - if !com.IsFile(patchPath) { - log.Trace("PullRequest[%d].testPatch: ignored corrupted data", pr.ID) - return nil - } - - RepoWorkingPool.CheckIn(com.ToStr(pr.BaseRepoID)) - defer RepoWorkingPool.CheckOut(com.ToStr(pr.BaseRepoID)) - - log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath) - - pr.Status = PullRequestStatusChecking - - indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())) - defer os.Remove(indexTmpPath) - - _, err = git.NewCommand("read-tree", pr.BaseBranch).RunInDirWithEnv("", []string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath}) - if err != nil { - return fmt.Errorf("git read-tree --index-output=%s %s: %v", indexTmpPath, pr.BaseBranch, err) - } - - prUnit, err := pr.BaseRepo.getUnit(e, UnitTypePullRequests) - if err != nil { - return err - } - prConfig := prUnit.PullRequestsConfig() - - args := []string{"apply", "--check", "--cached"} - if prConfig.IgnoreWhitespaceConflicts { - args = append(args, "--ignore-whitespace") - } - args = append(args, patchPath) - pr.ConflictedFiles = []string{} - - stderrBuilder := new(strings.Builder) - err = git.NewCommand(args...).RunInDirTimeoutEnvPipeline( - []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}, - -1, - "", - nil, - stderrBuilder) - stderr := stderrBuilder.String() - - if err != nil { - for i := range patchConflicts { - if strings.Contains(stderr, patchConflicts[i]) { - log.Trace("PullRequest[%d].testPatch (apply): has conflict: %s", pr.ID, stderr) - const prefix = "error: patch failed:" - pr.Status = PullRequestStatusConflict - pr.ConflictedFiles = make([]string, 0, 5) - scanner := bufio.NewScanner(strings.NewReader(stderr)) - for scanner.Scan() { - line := scanner.Text() - - if strings.HasPrefix(line, prefix) { - var found bool - var filepath = strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) - for _, f := range pr.ConflictedFiles { - if f == filepath { - found = true - break - } - } - if !found { - pr.ConflictedFiles = append(pr.ConflictedFiles, filepath) - } - } - // only list 10 conflicted files - if len(pr.ConflictedFiles) >= 10 { - break - } - } - - if len(pr.ConflictedFiles) > 0 { - log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) - } - - return nil - } - } - - return fmt.Errorf("git apply --check: %v - %s", err, stderr) - } - return nil -} - // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) { +func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { // Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887 i := 0 for { - if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch); err == nil { + if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr); err == nil { return nil } if !IsErrNewIssueInsert(err) { @@ -613,7 +494,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err) } -func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) { +func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { @@ -635,20 +516,6 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid pr.Index = pull.Index pr.BaseRepo = repo - pr.Status = PullRequestStatusChecking - if len(patch) > 0 { - if err = repo.savePatch(sess, pr.Index, patch); err != nil { - return fmt.Errorf("SavePatch: %v", err) - } - - if err = pr.testPatch(sess); err != nil { - return fmt.Errorf("testPatch: %v", err) - } - } - // No conflict appears after test means mergeable. - if pr.Status == PullRequestStatusChecking { - pr.Status = PullRequestStatusMergeable - } pr.IssueID = pull.ID if _, err = sess.Insert(pr); err != nil { @@ -764,54 +631,6 @@ func (pr *PullRequest) UpdateCols(cols ...string) error { return err } -// UpdatePatch generates and saves a new patch. -func (pr *PullRequest) UpdatePatch() (err error) { - if err = pr.GetHeadRepo(); err != nil { - return fmt.Errorf("GetHeadRepo: %v", err) - } else if pr.HeadRepo == nil { - log.Trace("PullRequest[%d].UpdatePatch: ignored corrupted data", pr.ID) - return nil - } - - if err = pr.GetBaseRepo(); err != nil { - return fmt.Errorf("GetBaseRepo: %v", err) - } - - headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) - if err != nil { - return fmt.Errorf("OpenRepository: %v", err) - } - defer headGitRepo.Close() - - // Add a temporary remote. - tmpRemote := com.ToStr(time.Now().UnixNano()) - if err = headGitRepo.AddRemote(tmpRemote, RepoPath(pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name), true); err != nil { - return fmt.Errorf("AddRemote: %v", err) - } - defer func() { - if err := headGitRepo.RemoveRemote(tmpRemote); err != nil { - log.Error("UpdatePatch: RemoveRemote: %s", err) - } - }() - pr.MergeBase, _, err = headGitRepo.GetMergeBase(tmpRemote, pr.BaseBranch, pr.HeadBranch) - if err != nil { - return fmt.Errorf("GetMergeBase: %v", err) - } else if err = pr.Update(); err != nil { - return fmt.Errorf("Update: %v", err) - } - - patch, err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch) - if err != nil { - return fmt.Errorf("GetPatch: %v", err) - } - - if err = pr.BaseRepo.SavePatch(pr.Index, patch); err != nil { - return fmt.Errorf("BaseRepo.SavePatch: %v", err) - } - - return nil -} - // PushToBaseRepo pushes commits from branches of head repository to // corresponding branches of base repository. // FIXME: Only push branches that are actually updates? diff --git a/models/pull_test.go b/models/pull_test.go index 4971ff2e52..6f799c1c82 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -190,8 +190,6 @@ func TestPullRequest_UpdateCols(t *testing.T) { CheckConsistencyFor(t, pr) } -// TODO TestPullRequest_UpdatePatch - // TODO TestPullRequest_PushToBaseRepo func TestPullRequestList_LoadAttributes(t *testing.T) { diff --git a/models/repo.go b/models/repo.go index c904449bbd..09735bd2c2 100644 --- a/models/repo.go +++ b/models/repo.go @@ -887,42 +887,6 @@ func (repo *Repository) DescriptionHTML() template.HTML { return template.HTML(markup.Sanitize(string(desc))) } -// PatchPath returns corresponding patch file path of repository by given issue ID. -func (repo *Repository) PatchPath(index int64) (string, error) { - return repo.patchPath(x, index) -} - -func (repo *Repository) patchPath(e Engine, index int64) (string, error) { - if err := repo.getOwner(e); err != nil { - return "", err - } - - return filepath.Join(RepoPath(repo.Owner.Name, repo.Name), "pulls", com.ToStr(index)+".patch"), nil -} - -// SavePatch saves patch data to corresponding location by given issue ID. -func (repo *Repository) SavePatch(index int64, patch []byte) error { - return repo.savePatch(x, index, patch) -} - -func (repo *Repository) savePatch(e Engine, index int64, patch []byte) error { - patchPath, err := repo.patchPath(e, index) - if err != nil { - return fmt.Errorf("PatchPath: %v", err) - } - dir := filepath.Dir(patchPath) - - if err := os.MkdirAll(dir, os.ModePerm); err != nil { - return fmt.Errorf("Failed to create dir %s: %v", dir, err) - } - - if err = ioutil.WriteFile(patchPath, patch, 0644); err != nil { - return fmt.Errorf("WriteFile: %v", err) - } - - return nil -} - func isRepositoryExist(e Engine, u *User, repoName string) (bool, error) { has, err := e.Get(&Repository{ OwnerID: u.ID, diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 677201c5e0..53b8af4bb4 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -6,7 +6,6 @@ package git import ( - "bytes" "container/list" "fmt" "io" @@ -94,19 +93,22 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string) return compareInfo, nil } -// GetPatch generates and returns patch data between given revisions. -func (repo *Repository) GetPatch(base, head string) ([]byte, error) { - return NewCommand("diff", "-p", "--binary", base, head).RunInDirBytes(repo.Path) -} - -// GetFormatPatch generates and returns format-patch data between given revisions. -func (repo *Repository) GetFormatPatch(base, head string) (io.Reader, error) { - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - - if err := NewCommand("format-patch", "--binary", "--stdout", base+"..."+head). - RunInDirPipeline(repo.Path, stdout, stderr); err != nil { - return nil, concatenateError(err, stderr.String()) +// GetDiffOrPatch generates either diff or formatted patch data between given revisions +func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, formatted bool) error { + if formatted { + return repo.GetPatch(base, head, w) } - return stdout, nil + return repo.GetDiff(base, head, w) +} + +// GetDiff generates and returns patch data between given revisions. +func (repo *Repository) GetDiff(base, head string, w io.Writer) error { + return NewCommand("diff", "-p", "--binary", base, head). + RunInDirPipeline(repo.Path, w, nil) +} + +// GetPatch generates and returns format-patch data between given revisions. +func (repo *Repository) GetPatch(base, head string, w io.Writer) error { + return NewCommand("format-patch", "--binary", "--stdout", base+"..."+head). + RunInDirPipeline(repo.Path, w, nil) } diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index def67fa87b..bf4631d853 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -5,6 +5,7 @@ package git import ( + "bytes" "io/ioutil" "os" "path/filepath" @@ -21,7 +22,8 @@ func TestGetFormatPatch(t *testing.T) { repo, err := OpenRepository(clonedPath) assert.NoError(t, err) defer repo.Close() - rd, err := repo.GetFormatPatch("8d92fc95^", "8d92fc95") + rd := &bytes.Buffer{} + err = repo.GetPatch("8d92fc95^", "8d92fc95", rd) assert.NoError(t, err) patchb, err := ioutil.ReadAll(rd) assert.NoError(t, err) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9abcaa0496..93fa6ad276 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -244,12 +244,6 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption milestoneID = milestone.ID } - patch, err := headGitRepo.GetPatch(compareInfo.MergeBase, headBranch) - if err != nil { - ctx.Error(500, "GetPatch", err) - return - } - var deadlineUnix timeutil.TimeStamp if form.Deadline != nil { deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) @@ -306,7 +300,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption } } - if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, patch, assigneeIDs); err != nil { + if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, assigneeIDs); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err) return diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 5d5aaca253..adafb64eb3 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1282,11 +1282,6 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { // Regenerate patch and test conflict. if pr == nil { - if err = issue.PullRequest.UpdatePatch(); err != nil { - ctx.ServerError("UpdatePatch", err) - return - } - pull_service.AddToTaskQueue(issue.PullRequest) } } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 78406de8ac..c791bc55d9 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -11,7 +11,6 @@ import ( "crypto/subtle" "fmt" "html" - "io" "path" "strings" @@ -785,12 +784,6 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - patch, err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch) - if err != nil { - ctx.ServerError("GetPatch", err) - return - } - pullIssue := &models.Issue{ RepoID: repo.ID, Title: form.Title, @@ -813,7 +806,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt // instead of 500. - if err := pull_service.NewPullRequest(repo, pullIssue, labelIDs, attachments, pullRequest, patch, assigneeIDs); err != nil { + if err := pull_service.NewPullRequest(repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error()) return @@ -981,44 +974,16 @@ func CleanUpPullRequest(ctx *context.Context) { // DownloadPullDiff render a pull's raw diff func DownloadPullDiff(ctx *context.Context) { - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - if err != nil { - if models.IsErrIssueNotExist(err) { - ctx.NotFound("GetIssueByIndex", err) - } else { - ctx.ServerError("GetIssueByIndex", err) - } - return - } - - // Return not found if it's not a pull request - if !issue.IsPull { - ctx.NotFound("DownloadPullDiff", - fmt.Errorf("Issue is not a pull request")) - return - } - - if err = issue.LoadPullRequest(); err != nil { - ctx.ServerError("LoadPullRequest", err) - return - } - - pr := issue.PullRequest - if err = pr.GetBaseRepo(); err != nil { - ctx.ServerError("GetBaseRepo", err) - return - } - patch, err := pr.BaseRepo.PatchPath(pr.Index) - if err != nil { - ctx.ServerError("PatchPath", err) - return - } - - ctx.ServeFileContent(patch) + DownloadPullDiffOrPatch(ctx, false) } // DownloadPullPatch render a pull's raw patch func DownloadPullPatch(ctx *context.Context) { + DownloadPullDiffOrPatch(ctx, true) +} + +// DownloadPullDiffOrPatch render a pull's raw diff or patch +func DownloadPullDiffOrPatch(ctx *context.Context, patch bool) { issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -1042,27 +1007,9 @@ func DownloadPullPatch(ctx *context.Context) { } pr := issue.PullRequest - if err = pr.GetHeadRepo(); err != nil { - ctx.ServerError("GetHeadRepo", err) - return - } - headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) - if err != nil { - ctx.ServerError("OpenRepository", err) - return - } - defer headGitRepo.Close() - - patch, err := headGitRepo.GetFormatPatch(pr.MergeBase, pr.HeadBranch) - if err != nil { - ctx.ServerError("GetFormatPatch", err) - return - } - - _, err = io.Copy(ctx, patch) - if err != nil { - ctx.ServerError("io.Copy", err) + if err := pull_service.DownloadDiffOrPatch(pr, ctx, patch); err != nil { + ctx.ServerError("DownloadDiffOrPatch", err) return } } diff --git a/services/pull/check.go b/services/pull/check.go index 0fd3e2a76f..fc2ac927b8 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -170,7 +170,7 @@ func TestPullRequests() { if manuallyMerged(pr) { continue } - if err := pr.TestPatch(); err != nil { + if err := TestPatch(pr); err != nil { log.Error("testPatch: %v", err) continue } @@ -194,7 +194,13 @@ func TestPullRequests() { continue } else if manuallyMerged(pr) { continue - } else if err = pr.TestPatch(); err != nil { + } + pr.Status = models.PullRequestStatusChecking + if err := pr.Update(); err != nil { + log.Error("testPatch[%d]: Unable to update status to Checking Status %v", pr.ID, err) + continue + } + if err = TestPatch(pr); err != nil { log.Error("testPatch[%d]: %v", pr.ID, err) continue } diff --git a/services/pull/merge.go b/services/pull/merge.go index e5563a89b9..9b75c5ffda 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -68,94 +68,22 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor }() // Clone base repo. - tmpBasePath, err := models.CreateTemporaryPath("merge") + tmpBasePath, err := createTemporaryRepo(pr) if err != nil { log.Error("CreateTemporaryPath: %v", err) return err } - defer func() { if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("Merge: RemoveTemporaryPath: %s", err) } }() - headRepoPath := pr.HeadRepo.RepoPath() - - if err := git.InitRepository(tmpBasePath, false); err != nil { - log.Error("git init tmpBasePath: %v", err) - return err - } - - remoteRepoName := "head_repo" baseBranch := "base" - - // Add head repo remote. - addCacheRepo := func(staging, cache string) error { - p := filepath.Join(staging, ".git", "objects", "info", "alternates") - f, err := os.OpenFile(p, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) - if err != nil { - log.Error("Could not create .git/objects/info/alternates file in %s: %v", staging, err) - return err - } - defer f.Close() - data := filepath.Join(cache, "objects") - if _, err := fmt.Fprintln(f, data); err != nil { - log.Error("Could not write to .git/objects/info/alternates file in %s: %v", staging, err) - return err - } - return nil - } - - if err := addCacheRepo(tmpBasePath, baseGitRepo.Path); err != nil { - log.Error("Unable to add base repository to temporary repo [%s -> %s]: %v", pr.BaseRepo.FullName(), tmpBasePath, err) - return fmt.Errorf("Unable to add base repository to temporary repo [%s -> tmpBasePath]: %v", pr.BaseRepo.FullName(), err) - } + trackingBranch := "tracking" + stagingBranch := "staging" var outbuf, errbuf strings.Builder - if err := git.NewCommand("remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseGitRepo.Path).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr.BaseRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), err, outbuf.String(), errbuf.String()) - } - outbuf.Reset() - errbuf.Reset() - - if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - } - outbuf.Reset() - errbuf.Reset() - - if err := git.NewCommand("symbolic-ref", "HEAD", git.BranchPrefix+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to set HEAD as base branch [%s]: %v\n%s\n%s", tmpBasePath, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("Unable to set HEAD as base branch [tmpBasePath]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) - } - outbuf.Reset() - errbuf.Reset() - - if err := addCacheRepo(tmpBasePath, headRepoPath); err != nil { - log.Error("Unable to add head repository to temporary repo [%s -> %s]: %v", pr.HeadRepo.FullName(), tmpBasePath, err) - return fmt.Errorf("Unable to head base repository to temporary repo [%s -> tmpBasePath]: %v", pr.HeadRepo.FullName(), err) - } - - if err := git.NewCommand("remote", "add", remoteRepoName, headRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr.HeadRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), err, outbuf.String(), errbuf.String()) - } - outbuf.Reset() - errbuf.Reset() - - trackingBranch := "tracking" - // Fetch head branch - if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, err, outbuf.String(), errbuf.String()) - } - outbuf.Reset() - errbuf.Reset() - - stagingBranch := "staging" // Enable sparse-checkout sparseCheckoutList, err := getDiffTree(tmpBasePath, baseBranch, trackingBranch) diff --git a/services/pull/patch.go b/services/pull/patch.go new file mode 100644 index 0000000000..cb8d014486 --- /dev/null +++ b/services/pull/patch.go @@ -0,0 +1,216 @@ +// Copyright 2019 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 pull + +import ( + "bufio" + "context" + "fmt" + "io" + "io/ioutil" + "os" + "strings" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" +) + +// DownloadDiff will write the patch for the pr to the writer +func DownloadDiff(pr *models.PullRequest, w io.Writer, patch bool) error { + return DownloadDiffOrPatch(pr, w, false) +} + +// DownloadPatch will write the patch for the pr to the writer +func DownloadPatch(pr *models.PullRequest, w io.Writer, patch bool) error { + return DownloadDiffOrPatch(pr, w, true) +} + +// DownloadDiffOrPatch will write the patch for the pr to the writer +func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error { + // Clone base repo. + tmpBasePath, err := createTemporaryRepo(pr) + if err != nil { + log.Error("CreateTemporaryPath: %v", err) + return err + } + defer func() { + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("DownloadDiff: RemoveTemporaryPath: %s", err) + } + }() + + gitRepo, err := git.OpenRepository(tmpBasePath) + if err != nil { + return fmt.Errorf("OpenRepository: %v", err) + } + defer gitRepo.Close() + + pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath) + if err != nil { + pr.MergeBase = "base" + } + pr.MergeBase = strings.TrimSpace(pr.MergeBase) + if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil { + log.Error("Unable to get patch file from %s to %s in %s/%s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name, err) + return fmt.Errorf("Unable to get patch file from %s to %s in %s/%s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name, err) + } + return nil +} + +var patchErrorSuffices = []string{ + ": already exists in index", + ": patch does not apply", + ": already exists in working directory", + "unrecognized input", +} + +// TestPatch will test whether a simple patch will apply +func TestPatch(pr *models.PullRequest) error { + // Clone base repo. + tmpBasePath, err := createTemporaryRepo(pr) + if err != nil { + log.Error("CreateTemporaryPath: %v", err) + return err + } + defer func() { + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("Merge: RemoveTemporaryPath: %s", err) + } + }() + + gitRepo, err := git.OpenRepository(tmpBasePath) + if err != nil { + return fmt.Errorf("OpenRepository: %v", err) + } + defer gitRepo.Close() + + pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath) + if err != nil { + var err2 error + pr.MergeBase, err2 = gitRepo.GetRefCommitID(git.BranchPrefix + "base") + if err2 != nil { + return fmt.Errorf("GetMergeBase: %v and can't find commit ID for base: %v", err, err2) + } + } + pr.MergeBase = strings.TrimSpace(pr.MergeBase) + tmpPatchFile, err := ioutil.TempFile("", "patch") + if err != nil { + log.Error("Unable to create temporary patch file! Error: %v", err) + return fmt.Errorf("Unable to create temporary patch file! Error: %v", err) + } + defer func() { + _ = os.Remove(tmpPatchFile.Name()) + }() + + if err := gitRepo.GetDiff(pr.MergeBase, "tracking", tmpPatchFile); err != nil { + tmpPatchFile.Close() + log.Error("Unable to get patch file from %s to %s in %s/%s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name, err) + return fmt.Errorf("Unable to get patch file from %s to %s in %s/%s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name, err) + } + stat, err := tmpPatchFile.Stat() + if err != nil { + tmpPatchFile.Close() + return fmt.Errorf("Unable to stat patch file: %v", err) + } + patchPath := tmpPatchFile.Name() + tmpPatchFile.Close() + + if stat.Size() == 0 { + log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) + pr.Status = models.PullRequestStatusMergeable + pr.ConflictedFiles = []string{} + return nil + } + + log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath) + + pr.Status = models.PullRequestStatusChecking + + _, err = git.NewCommand("read-tree", "base").RunInDir(tmpBasePath) + if err != nil { + return fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err) + } + + prUnit, err := pr.BaseRepo.GetUnit(models.UnitTypePullRequests) + if err != nil { + return err + } + prConfig := prUnit.PullRequestsConfig() + + args := []string{"apply", "--check", "--cached"} + if prConfig.IgnoreWhitespaceConflicts { + args = append(args, "--ignore-whitespace") + } + args = append(args, patchPath) + pr.ConflictedFiles = make([]string, 0, 5) + + stderrReader, stderrWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to open stderr pipe: %v", err) + return fmt.Errorf("Unable to open stderr pipe: %v", err) + } + defer func() { + _ = stderrReader.Close() + _ = stderrWriter.Close() + }() + conflict := false + err = git.NewCommand(args...). + RunInDirTimeoutEnvFullPipelineFunc( + nil, -1, tmpBasePath, + nil, stderrWriter, nil, + func(ctx context.Context, cancel context.CancelFunc) { + _ = stderrWriter.Close() + const prefix = "error: patch failed:" + const errorPrefix = "error: " + conflictMap := map[string]bool{} + + scanner := bufio.NewScanner(stderrReader) + for scanner.Scan() { + line := scanner.Text() + fmt.Printf("%s\n", line) + if strings.HasPrefix(line, prefix) { + conflict = true + filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) + conflictMap[filepath] = true + } else if strings.HasPrefix(line, errorPrefix) { + conflict = true + for _, suffix := range patchErrorSuffices { + if strings.HasSuffix(line, suffix) { + filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) + if filepath != "" { + conflictMap[filepath] = true + } + break + } + } + } + // only list 10 conflicted files + if len(conflictMap) >= 10 { + break + } + } + if len(conflictMap) > 0 { + pr.ConflictedFiles = make([]string, 0, len(conflictMap)) + for key := range conflictMap { + pr.ConflictedFiles = append(pr.ConflictedFiles, key) + } + } + _ = stderrReader.Close() + }) + + if err != nil { + if conflict { + pr.Status = models.PullRequestStatusConflict + log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) + return nil + } + return fmt.Errorf("git apply --check: %v", err) + } + pr.Status = models.PullRequestStatusMergeable + + return nil +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 2650dacc11..df44402ad8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -15,8 +15,12 @@ import ( ) // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(repo *models.Repository, pull *models.Issue, labelIDs []int64, uuids []string, pr *models.PullRequest, patch []byte, assigneeIDs []int64) error { - if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr, patch); err != nil { +func NewPullRequest(repo *models.Repository, pull *models.Issue, labelIDs []int64, uuids []string, pr *models.PullRequest, assigneeIDs []int64) error { + if err := TestPatch(pr); err != nil { + return err + } + + if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr); err != nil { return err } @@ -56,10 +60,7 @@ func checkForInvalidation(requests models.PullRequestList, repoID int64, doer *m func addHeadRepoTasks(prs []*models.PullRequest) { for _, pr := range prs { log.Trace("addHeadRepoTasks[%d]: composing new test task", pr.ID) - if err := pr.UpdatePatch(); err != nil { - log.Error("UpdatePatch: %v", err) - continue - } else if err := pr.PushToBaseRepo(); err != nil { + if err := pr.PushToBaseRepo(); err != nil { log.Error("PushToBaseRepo: %v", err) continue } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go new file mode 100644 index 0000000000..bb6ce2921e --- /dev/null +++ b/services/pull/temp_repo.go @@ -0,0 +1,152 @@ +// Copyright 2019 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 pull + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" +) + +func createTemporaryRepo(pr *models.PullRequest) (string, error) { + if err := pr.GetHeadRepo(); err != nil { + log.Error("GetHeadRepo: %v", err) + return "", fmt.Errorf("GetHeadRepo: %v", err) + } else if pr.HeadRepo == nil { + log.Error("Pr %d HeadRepo %d does not exist", pr.ID, pr.HeadRepoID) + return "", &models.ErrRepoNotExist{ + ID: pr.HeadRepoID, + } + } else if err := pr.GetBaseRepo(); err != nil { + log.Error("GetBaseRepo: %v", err) + return "", fmt.Errorf("GetBaseRepo: %v", err) + } else if pr.BaseRepo == nil { + log.Error("Pr %d BaseRepo %d does not exist", pr.ID, pr.BaseRepoID) + return "", &models.ErrRepoNotExist{ + ID: pr.BaseRepoID, + } + } else if err := pr.HeadRepo.GetOwner(); err != nil { + log.Error("HeadRepo.GetOwner: %v", err) + return "", fmt.Errorf("HeadRepo.GetOwner: %v", err) + } else if err := pr.BaseRepo.GetOwner(); err != nil { + log.Error("BaseRepo.GetOwner: %v", err) + return "", fmt.Errorf("BaseRepo.GetOwner: %v", err) + } + + // Clone base repo. + tmpBasePath, err := models.CreateTemporaryPath("pull") + if err != nil { + log.Error("CreateTemporaryPath: %v", err) + return "", err + } + + baseRepoPath := pr.BaseRepo.RepoPath() + headRepoPath := pr.HeadRepo.RepoPath() + + if err := git.InitRepository(tmpBasePath, false); err != nil { + log.Error("git init tmpBasePath: %v", err) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", err + } + + remoteRepoName := "head_repo" + baseBranch := "base" + + // Add head repo remote. + addCacheRepo := func(staging, cache string) error { + p := filepath.Join(staging, ".git", "objects", "info", "alternates") + f, err := os.OpenFile(p, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) + if err != nil { + log.Error("Could not create .git/objects/info/alternates file in %s: %v", staging, err) + return err + } + defer f.Close() + data := filepath.Join(cache, "objects") + if _, err := fmt.Fprintln(f, data); err != nil { + log.Error("Could not write to .git/objects/info/alternates file in %s: %v", staging, err) + return err + } + return nil + } + + if err := addCacheRepo(tmpBasePath, baseRepoPath); err != nil { + log.Error("Unable to add base repository to temporary repo [%s -> %s]: %v", pr.BaseRepo.FullName(), tmpBasePath, err) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to add base repository to temporary repo [%s -> tmpBasePath]: %v", pr.BaseRepo.FullName(), err) + } + + var outbuf, errbuf strings.Builder + if err := git.NewCommand("remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr.BaseRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + if err := git.NewCommand("symbolic-ref", "HEAD", git.BranchPrefix+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to set HEAD as base branch [%s]: %v\n%s\n%s", tmpBasePath, err, outbuf.String(), errbuf.String()) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to set HEAD as base branch [tmpBasePath]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + if err := addCacheRepo(tmpBasePath, headRepoPath); err != nil { + log.Error("Unable to add head repository to temporary repo [%s -> %s]: %v", pr.HeadRepo.FullName(), tmpBasePath, err) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to head base repository to temporary repo [%s -> tmpBasePath]: %v", pr.HeadRepo.FullName(), err) + } + + if err := git.NewCommand("remote", "add", remoteRepoName, headRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr.HeadRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + trackingBranch := "tracking" + // Fetch head branch + if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) + if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { + log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) + } + return "", fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + return tmpBasePath, nil +}