Skip to content

Commit 57cca44

Browse files
zeripathzeripath
authored
Generate Diff and Patch direct from Pull head (#10936)
* Generate Diff and Patch direct from Pull head Fix #10932 Also fix "Empty Diff/Patch File when pull is merged" Closes #10934 Signed-off-by: Andrew Thornton <art27@cantab.net> * Add tests to ensure that diff does not change Signed-off-by: Andrew Thornton <art27@cantab.net> * Ensure diffs and pulls pages work if head branch is deleted too Signed-off-by: Andrew Thornton <art27@cantab.net>
1 parent f685edf commit 57cca44

File tree

2 files changed

+70
-20
lines changed

2 files changed

+70
-20
lines changed

‎integrations/git_test.go‎

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ func testGit(t *testing.T, u *url.URL) {
7171
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath))
7272
t.Run("MergeFork", func(t *testing.T) {
7373
t.Run("CreatePRAndMerge", doMergeFork(httpContext, forkedUserCtx, "master", httpContext.Username+":master"))
74-
t.Run("DeleteRepository", doAPIDeleteRepository(httpContext))
7574
rawTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS)
7675
mediaTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS)
7776
})
@@ -111,7 +110,6 @@ func testGit(t *testing.T, u *url.URL) {
111110
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath))
112111
t.Run("MergeFork", func(t *testing.T) {
113112
t.Run("CreatePRAndMerge", doMergeFork(sshContext, forkedUserCtx, "master", sshContext.Username+":master"))
114-
t.Run("DeleteRepository", doAPIDeleteRepository(sshContext))
115113
rawTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS)
116114
mediaTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS)
117115
})
@@ -419,8 +417,62 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun
419417
pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t)
420418
assert.NoError(t, err)
421419
})
420+
t.Run("EnsureCanSeePull", func(t *testing.T) {
421+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
422+
ctx.Session.MakeRequest(t, req, http.StatusOK)
423+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
424+
ctx.Session.MakeRequest(t, req, http.StatusOK)
425+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
426+
ctx.Session.MakeRequest(t, req, http.StatusOK)
427+
})
428+
var diffStr string
429+
t.Run("GetDiff", func(t *testing.T) {
430+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
431+
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
432+
diffStr = resp.Body.String()
433+
})
422434
t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index))
423-
435+
t.Run("EnsureCanSeePull", func(t *testing.T) {
436+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
437+
ctx.Session.MakeRequest(t, req, http.StatusOK)
438+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
439+
ctx.Session.MakeRequest(t, req, http.StatusOK)
440+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
441+
ctx.Session.MakeRequest(t, req, http.StatusOK)
442+
})
443+
t.Run("EnsureDiffNoChange", func(t *testing.T) {
444+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
445+
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
446+
assert.Equal(t, diffStr, resp.Body.String())
447+
})
448+
t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch))
449+
t.Run("EnsureCanSeePull", func(t *testing.T) {
450+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
451+
ctx.Session.MakeRequest(t, req, http.StatusOK)
452+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
453+
ctx.Session.MakeRequest(t, req, http.StatusOK)
454+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
455+
ctx.Session.MakeRequest(t, req, http.StatusOK)
456+
})
457+
t.Run("EnsureDiffNoChange", func(t *testing.T) {
458+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
459+
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
460+
assert.Equal(t, diffStr, resp.Body.String())
461+
})
462+
t.Run("DeleteRepository", doAPIDeleteRepository(ctx))
463+
t.Run("EnsureCanSeePull", func(t *testing.T) {
464+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
465+
ctx.Session.MakeRequest(t, req, http.StatusOK)
466+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
467+
ctx.Session.MakeRequest(t, req, http.StatusOK)
468+
req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
469+
ctx.Session.MakeRequest(t, req, http.StatusOK)
470+
})
471+
t.Run("EnsureDiffNoChange", func(t *testing.T) {
472+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index))
473+
resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
474+
assert.Equal(t, diffStr, resp.Body.String())
475+
})
424476
}
425477
}
426478

@@ -493,3 +545,14 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) {
493545
assert.True(t, repo.IsPrivate)
494546
}
495547
}
548+
549+
func doBranchDelete(ctx APITestContext, owner, repo, branch string) func(*testing.T) {
550+
return func(t *testing.T) {
551+
csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/branches", url.PathEscape(owner), url.PathEscape(repo)))
552+
553+
req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/branches/delete?name=%s", url.PathEscape(owner), url.PathEscape(repo), url.QueryEscape(branch)), map[string]string{
554+
"_csrf": csrf,
555+
})
556+
ctx.Session.MakeRequest(t, req, http.StatusOK)
557+
}
558+
}

‎services/pull/patch.go‎

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,30 +21,17 @@ import (
2121

2222
// DownloadDiffOrPatch will write the patch for the pr to the writer
2323
func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
24-
// Clone base repo.
25-
tmpBasePath, err := createTemporaryRepo(pr)
26-
if err != nil {
27-
log.Error("CreateTemporaryPath: %v", err)
24+
if err := pr.LoadBaseRepo(); err != nil {
25+
log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index, pr.ID)
2826
return err
2927
}
30-
defer func() {
31-
if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
32-
log.Error("DownloadDiff: RemoveTemporaryPath: %s", err)
33-
}
34-
}()
3528

36-
gitRepo, err := git.OpenRepository(tmpBasePath)
29+
gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
3730
if err != nil {
3831
return fmt.Errorf("OpenRepository: %v", err)
3932
}
4033
defer gitRepo.Close()
41-
42-
pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
43-
if err != nil {
44-
pr.MergeBase = "base"
45-
}
46-
pr.MergeBase = strings.TrimSpace(pr.MergeBase)
47-
if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil {
34+
if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
4835
log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
4936
return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
5037
}

0 commit comments

Comments
 (0)