Remove SavePatch and generate patches on the fly (#9302)

* Save patches to temporary files

* Remove SavePatch and generate patches on the fly

* Use ioutil.TempDir

* fixup! Use ioutil.TempDir

* fixup! fixup! Use ioutil.TempDir

* RemoveAll LocalCopyPath() in initIntergrationTest

* Default to status checking on PR creation

* Remove unnecessary set to StatusChecking

* Protect against unable to load repo

* Handle conflicts

* Restore original conflict setting

* In TestPullRequests update status to StatusChecking before running TestPatch

integrations/integration_test.go

@@ -125,6 +125,7 @@ func initIntegrationTest() {
 
 	setting.SetCustomPathAndConf("", "", "")
 	setting.NewContext()
+	os.RemoveAll(models.LocalCopyPath())
 	setting.CheckLFSVersion()
 	setting.InitDBConfig()
 
@@ -182,7 +183,6 @@ func prepareTestEnv(t testing.TB, skip ...int) func() {
 	deferFn := PrintCurrentTest(t, ourSkip)
 	assert.NoError(t, models.LoadFixtures())
 	assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
-	assert.NoError(t, os.RemoveAll(models.LocalCopyPath()))
 
 	assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
 		setting.RepoRootPath))

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
 }

models/issue_xref_test.go

@@ -142,8 +142,8 @@ func testCreatePR(t *testing.T, repo, doer int64, title, content string) *PullRe
 	r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository)
 	d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User)
 	i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: true}
-	pr := &PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base"}
-	assert.NoError(t, NewPullRequest(r, i, nil, nil, pr, nil))
+	pr := &PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base", Status: PullRequestStatusMergeable}
+	assert.NoError(t, NewPullRequest(r, i, nil, nil, pr))
 	pr.Issue = i
 	return pr
 }

models/pull.go

@@ -6,22 +6,16 @@
 package models
 
 import (
-	"bufio"
 	"fmt"
 	"os"
 	"path"
-	"path/filepath"
-	"strconv"
 	"strings"
-	"time"
 
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
-
-	"github.com/unknwon/com"
 )
 
 // PullRequestType defines pull request type
@@ -481,125 +475,12 @@ func (pr *PullRequest) SetMerged() (err error) {
 	return nil
 }
 
-// patchConflicts is a list of conflict description from Git.
-var patchConflicts = []string{
-	"patch does not apply",
-	"already exists in working directory",
-	"unrecognized input",
-	"error:",
-}
-
-// TestPatch checks if patch can be merged to base repository without conflict.
-func (pr *PullRequest) TestPatch() error {
-	return pr.testPatch(x)
-}
-
-// testPatch checks if patch can be merged to base repository without conflict.
-func (pr *PullRequest) testPatch(e Engine) (err error) {
-	if pr.BaseRepo == nil {
-		pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID)
-		if err != nil {
-			return fmt.Errorf("GetRepositoryByID: %v", err)
-		}
-	}
-
-	patchPath, err := pr.BaseRepo.patchPath(e, pr.Index)
-	if err != nil {
-		return fmt.Errorf("BaseRepo.PatchPath: %v", err)
-	}
-
-	// Fast fail if patch does not exist, this assumes data is corrupted.
-	if !com.IsFile(patchPath) {
-		log.Trace("PullRequest[%d].testPatch: ignored corrupted data", pr.ID)
-		return nil
-	}
-
-	RepoWorkingPool.CheckIn(com.ToStr(pr.BaseRepoID))
-	defer RepoWorkingPool.CheckOut(com.ToStr(pr.BaseRepoID))
-
-	log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
-
-	pr.Status = PullRequestStatusChecking
-
-	indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond()))
-	defer os.Remove(indexTmpPath)
-
-	_, err = git.NewCommand("read-tree", pr.BaseBranch).RunInDirWithEnv("", []string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath})
-	if err != nil {
-		return fmt.Errorf("git read-tree --index-output=%s %s: %v", indexTmpPath, pr.BaseBranch, err)
-	}
-
-	prUnit, err := pr.BaseRepo.getUnit(e, UnitTypePullRequests)
-	if err != nil {
-		return err
-	}
-	prConfig := prUnit.PullRequestsConfig()
-
-	args := []string{"apply", "--check", "--cached"}
-	if prConfig.IgnoreWhitespaceConflicts {
-		args = append(args, "--ignore-whitespace")
-	}
-	args = append(args, patchPath)
-	pr.ConflictedFiles = []string{}
-
-	stderrBuilder := new(strings.Builder)
-	err = git.NewCommand(args...).RunInDirTimeoutEnvPipeline(
-		[]string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()},
-		-1,
-		"",
-		nil,
-		stderrBuilder)
-	stderr := stderrBuilder.String()
-
-	if err != nil {
-		for i := range patchConflicts {
-			if strings.Contains(stderr, patchConflicts[i]) {
-				log.Trace("PullRequest[%d].testPatch (apply): has conflict: %s", pr.ID, stderr)
-				const prefix = "error: patch failed:"
-				pr.Status = PullRequestStatusConflict
-				pr.ConflictedFiles = make([]string, 0, 5)
-				scanner := bufio.NewScanner(strings.NewReader(stderr))
-				for scanner.Scan() {
-					line := scanner.Text()
-
-					if strings.HasPrefix(line, prefix) {
-						var found bool
-						var filepath = strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0])
-						for _, f := range pr.ConflictedFiles {
-							if f == filepath {
-								found = true
-								break
-							}
-						}
-						if !found {
-							pr.ConflictedFiles = append(pr.ConflictedFiles, filepath)
-						}
-					}
-					// only list 10 conflicted files
-					if len(pr.ConflictedFiles) >= 10 {
-						break
-					}
-				}
-
-				if len(pr.ConflictedFiles) > 0 {
-					log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
-				}
-
-				return nil
-			}
-		}
-
-		return fmt.Errorf("git apply --check: %v - %s", err, stderr)
-	}
-	return nil
-}
-
 // NewPullRequest creates new pull request with labels for repository.
-func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) {
+func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
 	// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
 	i := 0
 	for {
-		if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch); err == nil {
+		if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr); err == nil {
 			return nil
 		}
 		if !IsErrNewIssueInsert(err) {
@@ -613,7 +494,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 	return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err)
 }
 
-func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) {
+func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {
@@ -635,20 +516,6 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid
 
 	pr.Index = pull.Index
 	pr.BaseRepo = repo
-	pr.Status = PullRequestStatusChecking
-	if len(patch) > 0 {
-		if err = repo.savePatch(sess, pr.Index, patch); err != nil {
-			return fmt.Errorf("SavePatch: %v", err)
-		}
-
-		if err = pr.testPatch(sess); err != nil {
-			return fmt.Errorf("testPatch: %v", err)
-		}
-	}
-	// No conflict appears after test means mergeable.
-	if pr.Status == PullRequestStatusChecking {
-		pr.Status = PullRequestStatusMergeable
-	}
 
 	pr.IssueID = pull.ID
 	if _, err = sess.Insert(pr); err != nil {
@@ -764,54 +631,6 @@ func (pr *PullRequest) UpdateCols(cols ...string) error {
 	return err
 }
 
-// UpdatePatch generates and saves a new patch.
-func (pr *PullRequest) UpdatePatch() (err error) {
-	if err = pr.GetHeadRepo(); err != nil {
-		return fmt.Errorf("GetHeadRepo: %v", err)
-	} else if pr.HeadRepo == nil {
-		log.Trace("PullRequest[%d].UpdatePatch: ignored corrupted data", pr.ID)
-		return nil
-	}
-
-	if err = pr.GetBaseRepo(); err != nil {
-		return fmt.Errorf("GetBaseRepo: %v", err)
-	}
-
-	headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
-	if err != nil {
-		return fmt.Errorf("OpenRepository: %v", err)
-	}
-	defer headGitRepo.Close()
-
-	// Add a temporary remote.
-	tmpRemote := com.ToStr(time.Now().UnixNano())
-	if err = headGitRepo.AddRemote(tmpRemote, RepoPath(pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name), true); err != nil {
-		return fmt.Errorf("AddRemote: %v", err)
-	}
-	defer func() {
-		if err := headGitRepo.RemoveRemote(tmpRemote); err != nil {
-			log.Error("UpdatePatch: RemoveRemote: %s", err)
-		}
-	}()
-	pr.MergeBase, _, err = headGitRepo.GetMergeBase(tmpRemote, pr.BaseBranch, pr.HeadBranch)
-	if err != nil {
-		return fmt.Errorf("GetMergeBase: %v", err)
-	} else if err = pr.Update(); err != nil {
-		return fmt.Errorf("Update: %v", err)
-	}
-
-	patch, err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch)
-	if err != nil {
-		return fmt.Errorf("GetPatch: %v", err)
-	}
-
-	if err = pr.BaseRepo.SavePatch(pr.Index, patch); err != nil {
-		return fmt.Errorf("BaseRepo.SavePatch: %v", err)
-	}
-
-	return nil
-}
-
 // PushToBaseRepo pushes commits from branches of head repository to
 // corresponding branches of base repository.
 // FIXME: Only push branches that are actually updates?

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) {

models/repo.go

@@ -887,42 +887,6 @@ func (repo *Repository) DescriptionHTML() template.HTML {
 	return template.HTML(markup.Sanitize(string(desc)))
 }
 
-// PatchPath returns corresponding patch file path of repository by given issue ID.
-func (repo *Repository) PatchPath(index int64) (string, error) {
-	return repo.patchPath(x, index)
-}
-
-func (repo *Repository) patchPath(e Engine, index int64) (string, error) {
-	if err := repo.getOwner(e); err != nil {
-		return "", err
-	}
-
-	return filepath.Join(RepoPath(repo.Owner.Name, repo.Name), "pulls", com.ToStr(index)+".patch"), nil
-}
-
-// SavePatch saves patch data to corresponding location by given issue ID.
-func (repo *Repository) SavePatch(index int64, patch []byte) error {
-	return repo.savePatch(x, index, patch)
-}
-
-func (repo *Repository) savePatch(e Engine, index int64, patch []byte) error {
-	patchPath, err := repo.patchPath(e, index)
-	if err != nil {
-		return fmt.Errorf("PatchPath: %v", err)
-	}
-	dir := filepath.Dir(patchPath)
-
-	if err := os.MkdirAll(dir, os.ModePerm); err != nil {
-		return fmt.Errorf("Failed to create dir %s: %v", dir, err)
-	}
-
-	if err = ioutil.WriteFile(patchPath, patch, 0644); err != nil {
-		return fmt.Errorf("WriteFile: %v", err)
-	}
-
-	return nil
-}
-
 func isRepositoryExist(e Engine, u *User, repoName string) (bool, error) {
 	has, err := e.Get(&Repository{
 		OwnerID:   u.ID,

modules/git/repo_compare.go

@@ -6,7 +6,6 @@
 package git
 
 import (
-	"bytes"
 	"container/list"
 	"fmt"
 	"io"
@@ -94,19 +93,22 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string)
 	return compareInfo, nil
 }
 
-// GetPatch generates and returns patch data between given revisions.
-func (repo *Repository) GetPatch(base, head string) ([]byte, error) {
-	return NewCommand("diff", "-p", "--binary", base, head).RunInDirBytes(repo.Path)
+// 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)
 }