From 8ff386b8eef965e87f3780b497d25535e8e95556 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 8 Dec 2019 21:51:06 +0000 Subject: [PATCH 01/12] Save patches to temporary files --- models/issue_xref_test.go | 2 +- models/pull.go | 28 ++++++++++++++++++++-------- models/repo.go | 24 ++++++++++++++++++------ modules/git/repo_compare.go | 4 ++-- routers/api/v1/repo/pull.go | 23 +++++++++++++++++++++-- routers/repo/pull.go | 23 +++++++++++++++++++++-- services/pull/pull.go | 4 ++-- 7 files changed, 85 insertions(+), 23 deletions(-) diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index c13577e905ae..d7a0a88f78ab 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -143,7 +143,7 @@ func testCreatePR(t *testing.T, repo, doer int64, title, content string) *PullRe 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)) + assert.NoError(t, NewPullRequest(r, i, nil, nil, pr, 0, "unknown")) pr.Issue = i return pr } diff --git a/models/pull.go b/models/pull.go index 2bd79202f094..82fb80e82b9c 100644 --- a/models/pull.go +++ b/models/pull.go @@ -8,6 +8,7 @@ package models import ( "bufio" "fmt" + "io/ioutil" "os" "path" "path/filepath" @@ -595,11 +596,11 @@ func (pr *PullRequest) testPatch(e Engine) (err error) { } // 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, patchFileSize int64, patchFileName string) (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, patchFileSize, patchFileName); err == nil { return nil } if !IsErrNewIssueInsert(err) { @@ -613,7 +614,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, patchFileSize int64, patchFileName string) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { @@ -636,8 +637,8 @@ 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 { + if patchFileSize > 0 { + if err = repo.savePatch(sess, pr.Index, patchFileName); err != nil { return fmt.Errorf("SavePatch: %v", err) } @@ -800,12 +801,23 @@ func (pr *PullRequest) UpdatePatch() (err error) { return fmt.Errorf("Update: %v", err) } - patch, err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch) + tmpPatchFile, err := ioutil.TempFile("", "patch") if err != nil { - return fmt.Errorf("GetPatch: %v", err) + 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 := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, 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) } - if err = pr.BaseRepo.SavePatch(pr.Index, patch); err != nil { + tmpPatchFile.Close() + if err = pr.BaseRepo.SavePatch(pr.Index, tmpPatchFile.Name()); err != nil { return fmt.Errorf("BaseRepo.SavePatch: %v", err) } diff --git a/models/repo.go b/models/repo.go index e809bafa309f..d694b6aaeedb 100644 --- a/models/repo.go +++ b/models/repo.go @@ -15,6 +15,7 @@ import ( // Needed for jpeg support _ "image/jpeg" "image/png" + "io" "io/ioutil" "net/url" "os" @@ -901,11 +902,11 @@ func (repo *Repository) patchPath(e Engine, index int64) (string, error) { } // 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(index int64, name string) error { + return repo.savePatch(x, index, name) } -func (repo *Repository) savePatch(e Engine, index int64, patch []byte) error { +func (repo *Repository) savePatch(e Engine, index int64, name string) error { patchPath, err := repo.patchPath(e, index) if err != nil { return fmt.Errorf("PatchPath: %v", err) @@ -916,10 +917,21 @@ func (repo *Repository) savePatch(e Engine, index int64, patch []byte) error { 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) + inputFile, err := os.Open(name) + if err != nil { + return fmt.Errorf("Couldn't open temporary patch file: %s", err) + } + outputFile, err := os.Create(patchPath) + if err != nil { + inputFile.Close() + return fmt.Errorf("Couldn't open destination patch file: %s", err) + } + defer outputFile.Close() + _, err = io.Copy(outputFile, inputFile) + inputFile.Close() + if err != nil { + return fmt.Errorf("Writing to patch file failed: %s", err) } - return nil } diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 677201c5e035..383af0a8c4a5 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -95,8 +95,8 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string) } // 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) +func (repo *Repository) GetPatch(base, head string, w io.Writer) error { + return NewCommand("diff", "-p", "--binary", base, head).RunInDirPipeline(repo.Path, w, nil) } // GetFormatPatch generates and returns format-patch data between given revisions. diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9abcaa0496a1..c0c22ce5d4ac 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -6,7 +6,9 @@ package repo import ( "fmt" + "io/ioutil" "net/http" + "os" "strings" "time" @@ -244,12 +246,29 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption milestoneID = milestone.ID } - patch, err := headGitRepo.GetPatch(compareInfo.MergeBase, headBranch) + tmpPatchFile, err := ioutil.TempFile("", "patch") if err != nil { + ctx.Error(500, "CreateTemporaryFile", err) + return + } + defer func() { + _ = os.Remove(tmpPatchFile.Name()) + }() + + if err := headGitRepo.GetPatch(compareInfo.MergeBase, headBranch, tmpPatchFile); err != nil { + tmpPatchFile.Close() ctx.Error(500, "GetPatch", err) return } + stat, err := tmpPatchFile.Stat() + if err != nil { + tmpPatchFile.Close() + ctx.Error(500, "StatPatch", err) + return + } + + tmpPatchFile.Close() var deadlineUnix timeutil.TimeStamp if form.Deadline != nil { deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) @@ -306,7 +325,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, stat.Size(), tmpPatchFile.Name(), assigneeIDs); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err) return diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 78406de8acdc..8221e49fc9e8 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -12,6 +12,8 @@ import ( "fmt" "html" "io" + "io/ioutil" + "os" "path" "strings" @@ -785,12 +787,29 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - patch, err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch) + tmpPatchFile, err := ioutil.TempFile("", "patch") if err != nil { + ctx.ServerError("CreateTemporaryFile", err) + return + } + defer func() { + _ = os.Remove(tmpPatchFile.Name()) + }() + + if err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch, tmpPatchFile); err != nil { + tmpPatchFile.Close() ctx.ServerError("GetPatch", err) return } + stat, err := tmpPatchFile.Stat() + if err != nil { + tmpPatchFile.Close() + ctx.ServerError("StatPatch", err) + return + } + tmpPatchFile.Close() + pullIssue := &models.Issue{ RepoID: repo.ID, Title: form.Title, @@ -813,7 +832,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, stat.Size(), tmpPatchFile.Name(), assigneeIDs); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error()) return diff --git a/services/pull/pull.go b/services/pull/pull.go index 2650dacc116d..648ecf864909 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -15,8 +15,8 @@ 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, patchFileSize int64, patchFileName string, assigneeIDs []int64) error { + if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr, patchFileSize, patchFileName); err != nil { return err } From f2f0a418e4cafa8aa71dd14bd1e3f4d2bcb2378e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 9 Dec 2019 14:58:11 +0000 Subject: [PATCH 02/12] Remove SavePatch and generate patches on the fly --- models/issue_xref_test.go | 4 +- models/pull.go | 199 +------------------------------ models/pull_test.go | 2 - models/repo.go | 48 -------- modules/git/repo_compare.go | 28 +++-- modules/git/repo_compare_test.go | 4 +- routers/api/v1/repo/pull.go | 27 +---- routers/repo/issue.go | 5 - routers/repo/pull.go | 90 ++------------ services/pull/check.go | 4 +- services/pull/merge.go | 78 +----------- services/pull/patch.go | 196 ++++++++++++++++++++++++++++++ services/pull/pull.go | 13 +- services/pull/temp_repo.go | 136 +++++++++++++++++++++ 14 files changed, 377 insertions(+), 457 deletions(-) create mode 100644 services/pull/patch.go create mode 100644 services/pull/temp_repo.go diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index d7a0a88f78ab..d8defd99c64a 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, 0, "unknown")) + 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 82fb80e82b9c..33adc3214ffa 100644 --- a/models/pull.go +++ b/models/pull.go @@ -6,23 +6,16 @@ package models import ( - "bufio" "fmt" - "io/ioutil" "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 @@ -482,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, patchFileSize int64, patchFileName string) (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, patchFileSize, patchFileName); err == nil { + if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr); err == nil { return nil } if !IsErrNewIssueInsert(err) { @@ -614,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, patchFileSize int64, patchFileName string) (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 { @@ -636,20 +516,6 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid pr.Index = pull.Index pr.BaseRepo = repo - pr.Status = PullRequestStatusChecking - if patchFileSize > 0 { - if err = repo.savePatch(sess, pr.Index, patchFileName); 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 { @@ -765,65 +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) - } - - 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 := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, 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) - } - - tmpPatchFile.Close() - if err = pr.BaseRepo.SavePatch(pr.Index, tmpPatchFile.Name()); 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 4971ff2e52e2..6f799c1c82f5 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 d694b6aaeedb..3dd1362f1092 100644 --- a/models/repo.go +++ b/models/repo.go @@ -15,7 +15,6 @@ import ( // Needed for jpeg support _ "image/jpeg" "image/png" - "io" "io/ioutil" "net/url" "os" @@ -888,53 +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, name string) error { - return repo.savePatch(x, index, name) -} - -func (repo *Repository) savePatch(e Engine, index int64, name string) 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) - } - - inputFile, err := os.Open(name) - if err != nil { - return fmt.Errorf("Couldn't open temporary patch file: %s", err) - } - outputFile, err := os.Create(patchPath) - if err != nil { - inputFile.Close() - return fmt.Errorf("Couldn't open destination patch file: %s", err) - } - defer outputFile.Close() - _, err = io.Copy(outputFile, inputFile) - inputFile.Close() - if err != nil { - return fmt.Errorf("Writing to patch file failed: %s", 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 383af0a8c4a5..53b8af4bb424 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, w io.Writer) error { - return NewCommand("diff", "-p", "--binary", base, head).RunInDirPipeline(repo.Path, w, nil) +// 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 repo.GetDiff(base, head, w) } -// 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) +// 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) +} - if err := NewCommand("format-patch", "--binary", "--stdout", base+"..."+head). - RunInDirPipeline(repo.Path, stdout, stderr); err != nil { - return nil, concatenateError(err, stderr.String()) - } - return stdout, 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 def67fa87b7f..bf4631d853ea 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 c0c22ce5d4ac..93fa6ad276a0 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -6,9 +6,7 @@ package repo import ( "fmt" - "io/ioutil" "net/http" - "os" "strings" "time" @@ -246,29 +244,6 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption milestoneID = milestone.ID } - tmpPatchFile, err := ioutil.TempFile("", "patch") - if err != nil { - ctx.Error(500, "CreateTemporaryFile", err) - return - } - defer func() { - _ = os.Remove(tmpPatchFile.Name()) - }() - - if err := headGitRepo.GetPatch(compareInfo.MergeBase, headBranch, tmpPatchFile); err != nil { - tmpPatchFile.Close() - ctx.Error(500, "GetPatch", err) - return - } - - stat, err := tmpPatchFile.Stat() - if err != nil { - tmpPatchFile.Close() - ctx.Error(500, "StatPatch", err) - return - } - - tmpPatchFile.Close() var deadlineUnix timeutil.TimeStamp if form.Deadline != nil { deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) @@ -325,7 +300,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption } } - if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, stat.Size(), tmpPatchFile.Name(), 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 5d5aaca253f2..adafb64eb3e4 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 8221e49fc9e8..c791bc55d9ab 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -11,9 +11,6 @@ import ( "crypto/subtle" "fmt" "html" - "io" - "io/ioutil" - "os" "path" "strings" @@ -787,29 +784,6 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - tmpPatchFile, err := ioutil.TempFile("", "patch") - if err != nil { - ctx.ServerError("CreateTemporaryFile", err) - return - } - defer func() { - _ = os.Remove(tmpPatchFile.Name()) - }() - - if err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch, tmpPatchFile); err != nil { - tmpPatchFile.Close() - ctx.ServerError("GetPatch", err) - return - } - - stat, err := tmpPatchFile.Stat() - if err != nil { - tmpPatchFile.Close() - ctx.ServerError("StatPatch", err) - return - } - tmpPatchFile.Close() - pullIssue := &models.Issue{ RepoID: repo.ID, Title: form.Title, @@ -832,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, stat.Size(), tmpPatchFile.Name(), 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 @@ -1000,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) { @@ -1061,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 0fd3e2a76f47..f59724ced622 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,7 @@ func TestPullRequests() { continue } else if manuallyMerged(pr) { continue - } else if err = pr.TestPatch(); err != nil { + } else 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 e5563a89b9e9..9b75c5ffda76 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -68,95 +68,23 @@ 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) - } - - 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" + var outbuf, errbuf strings.Builder + // Enable sparse-checkout sparseCheckoutList, err := getDiffTree(tmpBasePath, baseBranch, trackingBranch) if err != nil { diff --git a/services/pull/patch.go b/services/pull/patch.go new file mode 100644 index 000000000000..5a6a15474e84 --- /dev/null +++ b/services/pull/patch.go @@ -0,0 +1,196 @@ +// 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 +} + +// 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() + }() + 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:" + conflictMap := map[string]bool{} + + scanner := bufio.NewScanner(stderrReader) + for scanner.Scan() { + line := scanner.Text() + + if strings.HasPrefix(line, prefix) { + var filepath = strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) + conflictMap[filepath] = true + } + // 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) + } + pr.Status = models.PullRequestStatusConflict + } + _ = stderrReader.Close() + }) + + if err != nil { + 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", err) + } + pr.Status = models.PullRequestStatusMergeable + + return nil +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 648ecf864909..df44402ad81a 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, patchFileSize int64, patchFileName string, assigneeIDs []int64) error { - if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr, patchFileSize, patchFileName); 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 000000000000..10d2cf12a137 --- /dev/null +++ b/services/pull/temp_repo.go @@ -0,0 +1,136 @@ +// 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 err := pr.GetBaseRepo(); err != nil { + log.Error("GetBaseRepo: %v", err) + return "", fmt.Errorf("GetBaseRepo: %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 +} From 9e242843628a7d79a679cc142bac58caeab76dbc Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 9 Dec 2019 17:41:23 +0000 Subject: [PATCH 03/12] Use ioutil.TempDir --- models/helper_directory.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/models/helper_directory.go b/models/helper_directory.go index 813f0577bca0..1e239a487fb6 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 } From 94d73c2fa1dde783672a1a74665f712b9ec011f3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 9 Dec 2019 17:47:53 +0000 Subject: [PATCH 04/12] fixup! Use ioutil.TempDir --- models/helper_directory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/helper_directory.go b/models/helper_directory.go index 1e239a487fb6..8119ec91a362 100644 --- a/models/helper_directory.go +++ b/models/helper_directory.go @@ -29,7 +29,7 @@ func CreateTemporaryPath(prefix string) (string, error) { 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") + 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) From b5eb17a959b1e2e1b94a697f72636145ac1266bd Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 9 Dec 2019 18:21:37 +0000 Subject: [PATCH 05/12] fixup! fixup! Use ioutil.TempDir --- integrations/integration_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/integrations/integration_test.go b/integrations/integration_test.go index 4177493930b7..00ed7ce673fb 100644 --- a/integrations/integration_test.go +++ b/integrations/integration_test.go @@ -182,7 +182,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)) From 9d3f32cfad3f3c39c2b4ef64403eb24443201199 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 10 Dec 2019 08:45:57 +0000 Subject: [PATCH 06/12] RemoveAll LocalCopyPath() in initIntergrationTest --- integrations/integration_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integrations/integration_test.go b/integrations/integration_test.go index 00ed7ce673fb..5da9e04c7862 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() From 28cedf67dabad4979df4121ccbebf972fcd558b7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 10 Dec 2019 09:19:03 +0000 Subject: [PATCH 07/12] Default to status checking on PR creation --- models/pull.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/pull.go b/models/pull.go index 33adc3214ffa..a883c9bab4e4 100644 --- a/models/pull.go +++ b/models/pull.go @@ -516,6 +516,7 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid pr.Index = pull.Index pr.BaseRepo = repo + pr.Status = PullRequestStatusChecking pr.IssueID = pull.ID if _, err = sess.Insert(pr); err != nil { From 6ae5c4da7f717e96d32c1b958a05e7da30f1f264 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 10 Dec 2019 09:47:06 +0000 Subject: [PATCH 08/12] Remove unnecessary set to StatusChecking --- models/pull.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/pull.go b/models/pull.go index a883c9bab4e4..33adc3214ffa 100644 --- a/models/pull.go +++ b/models/pull.go @@ -516,7 +516,6 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid pr.Index = pull.Index pr.BaseRepo = repo - pr.Status = PullRequestStatusChecking pr.IssueID = pull.ID if _, err = sess.Insert(pr); err != nil { From 150832237f3172e8665831e111a6d24aec455189 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 11 Dec 2019 11:39:22 +0000 Subject: [PATCH 09/12] Protect against unable to load repo --- services/pull/temp_repo.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 10d2cf12a137..bb6ce2921ebe 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -20,9 +20,25 @@ 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. From ef1a9c9511afc256266aaeb49cff714fea8ef4df Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 11 Dec 2019 19:37:08 +0000 Subject: [PATCH 10/12] Handle conflicts --- services/pull/patch.go | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index 5a6a15474e84..cdbdb163aac5 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -61,6 +61,13 @@ func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error 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. @@ -150,6 +157,7 @@ func TestPatch(pr *models.PullRequest) error { _ = stderrReader.Close() _ = stderrWriter.Close() }() + conflict := false err = git.NewCommand(args...). RunInDirTimeoutEnvFullPipelineFunc( nil, -1, tmpBasePath, @@ -157,15 +165,28 @@ func TestPatch(pr *models.PullRequest) error { 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) { - var filepath = strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) + conflict = true + filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) conflictMap[filepath] = true + } else if strings.HasPrefix(line, errorPrefix) { + for _, suffix := range patchErrorSuffices { + if strings.HasSuffix(line, suffix) { + conflict = true + filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) + if filepath != "" { + conflictMap[filepath] = true + } + break + } + } } // only list 10 conflicted files if len(conflictMap) >= 10 { @@ -177,17 +198,16 @@ func TestPatch(pr *models.PullRequest) error { for key := range conflictMap { pr.ConflictedFiles = append(pr.ConflictedFiles, key) } - pr.Status = models.PullRequestStatusConflict } _ = stderrReader.Close() }) if err != nil { - if len(pr.ConflictedFiles) > 0 { + 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 From 696edba7caa61ed4f38f872ff0d811ec925dae56 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 11 Dec 2019 20:01:51 +0000 Subject: [PATCH 11/12] Restore original conflict setting --- services/pull/patch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index cdbdb163aac5..cb8d0144868d 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -177,9 +177,9 @@ func TestPatch(pr *models.PullRequest) error { 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) { - conflict = true filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) if filepath != "" { conflictMap[filepath] = true From 78b05ce1afeedb2a501b652da5b4a5931ab9f820 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 12 Dec 2019 11:06:06 +0000 Subject: [PATCH 12/12] In TestPullRequests update status to StatusChecking before running TestPatch --- services/pull/check.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/pull/check.go b/services/pull/check.go index f59724ced622..fc2ac927b8d5 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -194,7 +194,13 @@ func TestPullRequests() { continue } else if manuallyMerged(pr) { continue - } else if err = TestPatch(pr); 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 }