Skip to content

Save patches to temporary files #9296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Save patches to temporary files #9296

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
26d4e75
Save patches to temporary files
zeripath Dec 8, 2019
f6cfef0
Copy file instead
zeripath Dec 9, 2019
f86ac61
Import io
zeripath Dec 9, 2019
5d6d313
Always close tmpPatchFile
zeripath Dec 9, 2019
e349449
fmting
zeripath Dec 9, 2019
810898f
Apply suggestions from code review
zeripath Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/issue_xref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
28 changes: 20 additions & 8 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package models
import (
"bufio"
"fmt"
"io/ioutil"
"os"
"path"
"path/filepath"
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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 {
Copy link
Member

@guillep2k guillep2k Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the patch should be saved even if it's got 0 bytes. The reason is that a previous PR creation attempt (from a different PR, with patch data) could have failed between this step and the commit, so a left-over patch file (named after this same index number) might still be lingering in the directory. I hope that makes sense.

In fact, there are scenarios where that patch could be broken anyway with this retry mechanism. Mmm.... we should be locking that path, at the very least? Sorry, I wasn't aware that there were destinations other than the database when I wrote the retry code.

Copy link
Contributor Author

@zeripath zeripath Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make it simpler if we dropped that file size test.

Tbh I'm not certain that the patch files are ever actually cleaned up. I don't remember seeing any code looking at deleting them. I actually forgot to look explicitly - that's not like me!!

You're also right about the lack of file locking. This might be because we have generally migrated from the repoworkingcopy lock mechanism to rely on git locking but obviously this doesn't use that mechanism. #9302 obviously isn't affected by this.

I guess the question is how important is it provide a backport?

If we're not going to backport - then I'm happy to close this and move to the more radical refactor.

(It's not like this is the only place we still read in potentially huge amounts of data - diff and blame are both still doing that, and a change to streaming there will require huge refactoring.)

Copy link
Member

@guillep2k guillep2k Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, backporting is not my call, but I'm OK with not backporting this as long as #9302 makes it in time for 1.11.

return fmt.Errorf("SavePatch: %v", err)
}

Expand Down Expand Up @@ -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 {
Copy link
Member

@6543 6543 Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile); err != nil {
err = headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile)
tmpPatchFile.Close()
if err != nil {

tmpPatchFile.Close()
Copy link
Member

@6543 6543 Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
}

Expand Down
24 changes: 18 additions & 6 deletions models/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
// Needed for jpeg support
_ "image/jpeg"
"image/png"
"io"
"io/ioutil"
"net/url"
"os"
Expand Down Expand Up @@ -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)
Expand All @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions modules/git/repo_compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 21 additions & 2 deletions routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ package repo

import (
"fmt"
"io/ioutil"
"net/http"
"os"
"strings"
"time"

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down
23 changes: 21 additions & 2 deletions routers/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"fmt"
"html"
"io"
"io/ioutil"
"os"
"path"
"strings"

Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions services/pull/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down