From 26d4e75d3e9c542a9c648f725aec78051c083fa8 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 8 Dec 2019 21:51:06 +0000 Subject: [PATCH 1/6] Save patches to temporary files --- models/issue_xref_test.go | 2 +- models/pull.go | 26 ++++++++++++++++++-------- models/repo.go | 8 ++++---- modules/git/repo_compare.go | 4 ++-- routers/api/v1/repo/pull.go | 21 +++++++++++++++++++-- routers/repo/pull.go | 21 +++++++++++++++++++-- services/pull/pull.go | 4 ++-- 7 files changed, 65 insertions(+), 21 deletions(-) diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index c13577e905aeb..d7a0a88f78ab9 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 2bd79202f094b..b1375dc060afc 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,21 @@ 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 { + 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 { + 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 e809bafa309f1..e41269f695d17 100644 --- a/models/repo.go +++ b/models/repo.go @@ -901,11 +901,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,7 +916,7 @@ 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 { + if err := os.Rename(name, patchPath); err != nil { return fmt.Errorf("WriteFile: %v", err) } diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 677201c5e0358..383af0a8c4a5b 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 9abcaa0496a15..b7ea7999a76ea 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,27 @@ 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 { ctx.Error(500, "GetPatch", err) return } + stat, err := tmpPatchFile.Stat() + if err != nil { + ctx.Error(500, "StatPatch", err) + return + } + + tmpPatchFile.Close() var deadlineUnix timeutil.TimeStamp if form.Deadline != nil { deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) @@ -306,7 +323,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 78406de8acdc8..84073379161d8 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,27 @@ 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 { ctx.ServerError("GetPatch", err) return } + stat, err := tmpPatchFile.Stat() + if err != nil { + ctx.ServerError("StatPatch", err) + return + } + tmpPatchFile.Close() + pullIssue := &models.Issue{ RepoID: repo.ID, Title: form.Title, @@ -813,7 +830,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 2650dacc116da..648ecf8649098 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 f6cfef08bed46410b664f7a9c9fff5b4d3d4e789 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 9 Dec 2019 04:41:46 +0000 Subject: [PATCH 2/6] Copy file instead os.Rename cannot rename across disk boundaries --- models/repo.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/models/repo.go b/models/repo.go index e41269f695d17..1d7a36d08cbfd 100644 --- a/models/repo.go +++ b/models/repo.go @@ -916,10 +916,21 @@ func (repo *Repository) savePatch(e Engine, index int64, name string) error { return fmt.Errorf("Failed to create dir %s: %v", dir, err) } - if err := os.Rename(name, patchPath); 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 } From f86ac61db2d1d6359926983114cfb860792a6347 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 9 Dec 2019 04:50:15 +0000 Subject: [PATCH 3/6] Import io --- models/repo.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/repo.go b/models/repo.go index 1d7a36d08cbfd..d694b6aaeedbc 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" From 5d6d31334aadd0a7c6da91cbed52f85dfded21d6 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 9 Dec 2019 05:23:16 +0000 Subject: [PATCH 4/6] Always close tmpPatchFile --- models/pull.go | 2 ++ routers/api/v1/repo/pull.go | 2 ++ routers/repo/pull.go | 2 ++ 3 files changed, 6 insertions(+) diff --git a/models/pull.go b/models/pull.go index b1375dc060afc..82fb80e82b9c8 100644 --- a/models/pull.go +++ b/models/pull.go @@ -811,10 +811,12 @@ func (pr *PullRequest) UpdatePatch() (err error) { }() 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) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index b7ea7999a76ea..eee2e60c98720 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -256,12 +256,14 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption }() 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 } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 84073379161d8..82b0521e63b39 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -797,12 +797,14 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) }() 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 } From e3494493cb309c27252bc57161cf700924164877 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 9 Dec 2019 05:33:16 +0000 Subject: [PATCH 5/6] fmting --- routers/api/v1/repo/pull.go | 2 +- routers/repo/pull.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index eee2e60c98720..c73eb88ca5f6c 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -256,7 +256,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption }() if err := headGitRepo.GetPatch(compareInfo.MergeBase, headBranch, tmpPatchFile); err != nil { - tmpPatchFile.Close() + tmpPatchFile.Close() ctx.Error(500, "GetPatch", err) return } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 82b0521e63b39..a3a82d0399945 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -797,7 +797,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) }() if err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch, tmpPatchFile); err != nil { - tmpPatchFile.Close() + tmpPatchFile.Close() ctx.ServerError("GetPatch", err) return } From 810898f16404ef1b079c1c81bb5f1fd838596062 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 9 Dec 2019 05:42:44 +0000 Subject: [PATCH 6/6] Apply suggestions from code review --- routers/api/v1/repo/pull.go | 2 +- routers/repo/pull.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index c73eb88ca5f6c..c0c22ce5d4ac6 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -263,7 +263,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption stat, err := tmpPatchFile.Stat() if err != nil { - tmpPatchFile.Close() + tmpPatchFile.Close() ctx.Error(500, "StatPatch", err) return } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index a3a82d0399945..8221e49fc9e8e 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -804,7 +804,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) stat, err := tmpPatchFile.Stat() if err != nil { - tmpPatchFile.Close() + tmpPatchFile.Close() ctx.ServerError("StatPatch", err) return }