Skip to content

Commit 99a364a

Browse files
zeripathzeripath
authored
Generate Diff and Patch direct from Pull head (go-gitea#10936) (go-gitea#10938)
Backport go-gitea#10936 * Generate Diff and Patch direct from Pull head Fix go-gitea#10932 Also fix "Empty Diff/Patch File when pull is merged" Closes go-gitea#10934 * Add tests to ensure that diff does not change * Ensure diffs and pulls pages work if head branch is deleted too Signed-off-by: Andrew Thornton <[email protected]>
1 parent 3afbbfe commit 99a364a

File tree

2 files changed

+72
-22
lines changed

2 files changed

+72
-22
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: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,32 +31,19 @@ func DownloadPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
3131

3232
// DownloadDiffOrPatch will write the patch for the pr to the writer
3333
func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
34-
// Clone base repo.
35-
tmpBasePath, err := createTemporaryRepo(pr)
36-
if err != nil {
37-
log.Error("CreateTemporaryPath: %v", err)
34+
if err := pr.LoadBaseRepo(); err != nil {
35+
log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index, pr.ID)
3836
return err
3937
}
40-
defer func() {
41-
if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
42-
log.Error("DownloadDiff: RemoveTemporaryPath: %s", err)
43-
}
44-
}()
4538

46-
gitRepo, err := git.OpenRepository(tmpBasePath)
39+
gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
4740
if err != nil {
4841
return fmt.Errorf("OpenRepository: %v", err)
4942
}
5043
defer gitRepo.Close()
51-
52-
pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
53-
if err != nil {
54-
pr.MergeBase = "base"
55-
}
56-
pr.MergeBase = strings.TrimSpace(pr.MergeBase)
57-
if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil {
58-
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)
59-
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)
44+
if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
45+
log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
46+
return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
6047
}
6148
return nil
6249
}

0 commit comments

Comments
 (0)