Skip to content

Improve the performance of pull-request merging #641

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 2 commits into from
Closed

Improve the performance of pull-request merging #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8179ef8
Speedup pull-request mergeing
typeless Jan 11, 2017
c5babc2
Add a straightforward benchmark
typeless Sep 22, 2017
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
12 changes: 4 additions & 8 deletions integrations/editor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,7 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {

}

func testEditFile(t *testing.T, session *TestSession, user, repo, branch, filePath string) *TestResponse {

newContent := "Hello, World (Edited)\n"
func testEditFile(t testing.TB, session *TestSession, user, repo, branch, filePath, newContent string) *TestResponse {

// Get to the 'edit this file' page
req := NewRequest(t, "GET", path.Join(user, repo, "_edit", branch, filePath))
Expand Down Expand Up @@ -121,9 +119,7 @@ func testEditFile(t *testing.T, session *TestSession, user, repo, branch, filePa
return resp
}

func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, branch, targetBranch, filePath string) *TestResponse {

newContent := "Hello, World (Edited)\n"
func testEditFileToNewBranch(t testing.TB, session *TestSession, user, repo, branch, targetBranch, filePath, newContent string) *TestResponse {

// Get to the 'edit this file' page
req := NewRequest(t, "GET", path.Join(user, repo, "_edit", branch, filePath))
Expand Down Expand Up @@ -157,11 +153,11 @@ func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, bra
func TestEditFile(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
testEditFile(t, session, "user2", "repo1", "master", "README.md")
testEditFile(t, session, "user2", "repo1", "master", "README.md", "Hello, World (Edited)\n")
}

func TestEditFileToNewBranch(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "feature/test", "README.md")
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited)\n")
}
4 changes: 2 additions & 2 deletions integrations/pull_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"github.com/stretchr/testify/assert"
)

func testPullCreate(t *testing.T, session *TestSession, user, repo, branch string) *TestResponse {
func testPullCreate(t testing.TB, session *TestSession, user, repo, branch string) *TestResponse {
req := NewRequest(t, "GET", path.Join(user, repo))
resp := session.MakeRequest(t, req, http.StatusOK)

Expand Down Expand Up @@ -47,6 +47,6 @@ func TestPullCreate(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testEditFile(t, session, "user1", "repo1", "master", "README.md")
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
testPullCreate(t, session, "user1", "repo1", "master")
}
24 changes: 21 additions & 3 deletions integrations/pull_merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package integrations

import (
"fmt"
"net/http"
"path"
"strings"
Expand All @@ -13,7 +14,7 @@ import (
"github.com/stretchr/testify/assert"
)

func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string) *TestResponse {
func testPullMerge(t testing.TB, session *TestSession, user, repo, pullnum string) *TestResponse {
req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum))
resp := session.MakeRequest(t, req, http.StatusOK)

Expand Down Expand Up @@ -49,7 +50,7 @@ func TestPullMerge(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testEditFile(t, session, "user1", "repo1", "master", "README.md")
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")

resp := testPullCreate(t, session, "user1", "repo1", "master")

Expand All @@ -62,7 +63,7 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md")
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited)\n")

resp := testPullCreate(t, session, "user1", "repo1", "feature/test")

Expand Down Expand Up @@ -91,3 +92,20 @@ func TestPullCleanUpAfterMerge(t *testing.T) {

assert.EqualValues(t, "user1/feature/test has been deleted.", resultMsg)
}

func BenchmarkPullMerge(b *testing.B) {
prepareTestEnv(b)
session := loginUser(b, "user1")
testRepoFork(b, session)

for i := 0; i < b.N; i++ {
content := fmt.Sprintf("Hello, World (Edited) #%d\n", i)
testEditFile(b, session, "user1", "repo1", "master", "README.md", content)

resp := testPullCreate(b, session, "user1", "repo1", "master")

elem := strings.Split(RedirectURL(b, resp), "/")
assert.EqualValues(b, "pulls", elem[3])
testPullMerge(b, session, elem[1], elem[2], elem[4])
}
}
2 changes: 1 addition & 1 deletion integrations/repo_fork_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"github.com/stretchr/testify/assert"
)

func testRepoFork(t *testing.T, session *TestSession) *TestResponse {
func testRepoFork(t testing.TB, session *TestSession) *TestResponse {
// Step0: check the existence of the to-fork repo
req := NewRequest(t, "GET", "/user1/repo1")
resp := session.MakeRequest(t, req, http.StatusNotFound)
Expand Down
119 changes: 77 additions & 42 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,63 +270,98 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error
return fmt.Errorf("OpenRepository: %v", err)
}

// Clone base repo.
tmpBasePath := path.Join(setting.AppDataPath, "tmp/repos", com.ToStr(time.Now().Nanosecond())+".git")
baseRepoPath := pr.BaseRepo.RepoPath()

if err := os.MkdirAll(path.Dir(tmpBasePath), os.ModePerm); err != nil {
return fmt.Errorf("Failed to create dir %s: %v", tmpBasePath, err)
}
pullHead := fmt.Sprintf("refs/pull/%d/head", pr.Index)

defer os.RemoveAll(path.Dir(tmpBasePath))
// A temporary Git index file we are going to build. The merge commit will be based on it.
indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond()))
defer os.Remove(indexTmpPath)

var stderr string
if _, stderr, err = process.GetManager().ExecTimeout(5*time.Minute,
fmt.Sprintf("PullRequest.Merge (git clone): %s", tmpBasePath),
"git", "clone", baseGitRepo.Path, tmpBasePath); err != nil {
return fmt.Errorf("git clone: %s", stderr)
// A temporary Git working tree for git-merge-index and git-merge-one-file.
workTreeTmpPath, err := ioutil.TempDir("", "gitea-merge-")
Copy link
Member

Choose a reason for hiding this comment

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

Is not there missing +pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())?

Copy link
Contributor Author

@typeless typeless Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ioutil.TempDir will generate a suffix automatically. See https://golang.org/src/io/ioutil/tempfile.go?s=2138:2195#L66
Sorry, I see what you mean now. I'll fix it later.

Copy link
Contributor Author

@typeless typeless Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is not there missing +pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())?

This is actually correct despite a bit of inconsistent. I will clean this up a little bit.

The path indexTmpPath and workTreeTmpPath conrespond to GIT_INDEX_FILE and GIT_WORK_TREE respectively. I looked at this in a hurry last friday and got confused as well. 😆

if err != nil {
return fmt.Errorf("Cannot create temporary directory for git-merge-index")
}
defer os.RemoveAll(workTreeTmpPath)

// Check out base branch.
if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git checkout): %s", tmpBasePath),
"git", "checkout", pr.BaseBranch); err != nil {
return fmt.Errorf("git checkout: %s", stderr)
}
log.Trace("BaseRepo:%s Index:%s Ancestor:%s BaseBranch:%s HeadBranch:%s PullHead:%s",
baseRepoPath, indexTmpPath, pr.MergeBase, pr.BaseBranch, pr.HeadBranch, pullHead)

var stdout, stderr string

// Add head repo remote.
if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git remote add): %s", tmpBasePath),
"git", "remote", "add", "head_repo", headRepoPath); err != nil {
return fmt.Errorf("git remote add [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
// Build the index from the common ancestor, the base-branch head, and the pull head, for 3-way merge.
if _, stderr, err = process.GetManager().ExecDirEnv(-1, "",
fmt.Sprintf("PullRequest.Merge (git read-tree): %s", baseRepoPath),
[]string{"GIT_DIR=" + baseRepoPath, "GIT_INDEX_FILE=" + indexTmpPath},
"git", "read-tree", "-im", pr.MergeBase, pr.BaseBranch, pullHead); err != nil {
return fmt.Errorf("git read-tree -im %s %s %s: %s (%v)", pr.MergeBase, pr.BaseBranch, pullHead, stderr, err)
}

// Merge commits.
if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git fetch): %s", tmpBasePath),
"git", "fetch", "head_repo"); err != nil {
return fmt.Errorf("git fetch [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
// Run the merge algorithm throughout the prepared index.
if stdout, stderr, err = process.GetManager().ExecDirEnv(-1, "",
fmt.Sprintf("PullRequest.Merge (git merge-index)"),
[]string{
"GIT_DIR=" + baseRepoPath,
"GIT_INDEX_FILE=" + indexTmpPath,
"GIT_WORK_TREE=" + workTreeTmpPath,
},
"git", "merge-index", "git-merge-one-file", "-a"); err != nil {
return fmt.Errorf("git merge-index git merge-one-file -a: %s (%v)", stderr, err)
}

if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git merge --no-ff --no-commit): %s", tmpBasePath),
"git", "merge", "--no-ff", "--no-commit", "head_repo/"+pr.HeadBranch); err != nil {
return fmt.Errorf("git merge --no-ff --no-commit [%s]: %v - %s", tmpBasePath, err, stderr)
// Write the tree object back to the Git object store from the index.
if stdout, stderr, err = process.GetManager().ExecDirEnv(-1, "",
fmt.Sprintf("PullRequest.Merge (git write-tree): %s", baseRepoPath),
[]string{"GIT_DIR=" + baseRepoPath, "GIT_INDEX_FILE=" + indexTmpPath},
"git", "write-tree"); err != nil {
return fmt.Errorf("git write-tree: %s (%v)", stderr, err)
}
treeID := strings.TrimSpace(stdout)

sig := doer.NewGitSig()
if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git merge): %s", tmpBasePath),
"git", "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email),
"-m", fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch)); err != nil {
return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, stderr)
log.Trace("BaseRepo:%s TreeId:%s", baseRepoPath, treeID)

// Create the commit object based on the tree object we just built.
var headBranch *Branch
if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil {
return fmt.Errorf("Getting head branch: %s: %s (%v)", pr.HeadBranch, stderr, err)
}

// Push back to upstream.
if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
fmt.Sprintf("PullRequest.Merge (git push): %s", tmpBasePath),
"git", "push", baseGitRepo.Path, pr.BaseBranch); err != nil {
return fmt.Errorf("git push: %s", stderr)
var headCommit *git.Commit
if headCommit, err = headBranch.GetCommit(); err != nil {
return fmt.Errorf("Getting head commit: %s: %s (%v)", pr.HeadBranch, stderr, err)
}
commitMessage := fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch)

sig := doer.NewGitSig()
if stdout, stderr, err = process.GetManager().ExecDirEnv(-1, "",
fmt.Sprintf("PullRequest.Merge (git commit-tree): %s", baseRepoPath),
[]string{
"GIT_DIR=" + baseRepoPath,
"GIT_AUTHOR_NAME=" + headCommit.Author.Name,
"GIT_AUTHOR_EMAIL=" + headCommit.Author.Email,
"GIT_AUTHOR_DATE=" + headCommit.Author.When.Format("Mon, 02 Jan 2006 15:04:05 -0700"),
"GIT_COMMITTER_NAME=" + sig.Name,
"GIT_COMMITTER_EMAIL=" + sig.Email,
"GIT_COMMITTER_DATE=" + sig.When.Format("Mon, 02 Jan 2006 15:04:05 -0700"),
},
"git", "commit-tree", treeID, "-p", pr.BaseBranch, "-p", pullHead, "-m", commitMessage); err != nil {
return fmt.Errorf("git commit-tree %s -p %s -p %s: %s (%v)", strings.TrimSpace(treeID), pr.BaseBranch, pullHead, stderr, err)
}
commitID := strings.TrimSpace(stdout)

log.Trace("BaseRepo:%s CommitId:%s", baseRepoPath, commitID)

// Update the ref of the base-branch to point to the new commit object.
refPath := fmt.Sprintf("refs/heads/%s", pr.BaseBranch)
if _, stderr, err = process.GetManager().ExecDirEnv(-1, "",
fmt.Sprintf("PullRequest.Merge (git update-ref): %s", baseRepoPath),
[]string{"GIT_DIR=" + baseRepoPath},
"git", "update-ref", refPath, commitID); err != nil {
return fmt.Errorf("git update-ref -m %s %s: %s (%v)", commitMessage, refPath, stderr, err)
}

log.Trace("BaseRepo:%s Update-ref: %s->%s. Done.", baseRepoPath, refPath, commitID)

pr.MergedCommitID, err = baseGitRepo.GetBranchCommitID(pr.BaseBranch)
if err != nil {
Expand Down