From 57cca44c5e75025bc3d51bea028ca7012557f4f5 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 3 Apr 2020 14:21:41 +0100 Subject: [PATCH] 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 * Add tests to ensure that diff does not change Signed-off-by: Andrew Thornton * Ensure diffs and pulls pages work if head branch is deleted too Signed-off-by: Andrew Thornton --- integrations/git_test.go | 69 ++++++++++++++++++++++++++++++++++++++-- services/pull/patch.go | 21 +++--------- 2 files changed, 70 insertions(+), 20 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 8e02bc4db9..8813f34be7 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -71,7 +71,6 @@ func testGit(t *testing.T, u *url.URL) { t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath)) t.Run("MergeFork", func(t *testing.T) { t.Run("CreatePRAndMerge", doMergeFork(httpContext, forkedUserCtx, "master", httpContext.Username+":master")) - t.Run("DeleteRepository", doAPIDeleteRepository(httpContext)) rawTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS) mediaTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS) }) @@ -111,7 +110,6 @@ func testGit(t *testing.T, u *url.URL) { t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath)) t.Run("MergeFork", func(t *testing.T) { t.Run("CreatePRAndMerge", doMergeFork(sshContext, forkedUserCtx, "master", sshContext.Username+":master")) - t.Run("DeleteRepository", doAPIDeleteRepository(sshContext)) rawTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS) mediaTest(t, &forkedUserCtx, little, big, littleLFS, bigLFS) }) @@ -419,8 +417,62 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t) assert.NoError(t, err) }) + t.Run("EnsureCanSeePull", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + }) + var diffStr string + t.Run("GetDiff", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequest(t, req, http.StatusOK) + diffStr = resp.Body.String() + }) t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)) - + t.Run("EnsureCanSeePull", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + }) + t.Run("EnsureDiffNoChange", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, diffStr, resp.Body.String()) + }) + t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch)) + t.Run("EnsureCanSeePull", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + }) + t.Run("EnsureDiffNoChange", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, diffStr, resp.Body.String()) + }) + t.Run("DeleteRepository", doAPIDeleteRepository(ctx)) + t.Run("EnsureCanSeePull", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + }) + t.Run("EnsureDiffNoChange", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, diffStr, resp.Body.String()) + }) } } @@ -493,3 +545,14 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { assert.True(t, repo.IsPrivate) } } + +func doBranchDelete(ctx APITestContext, owner, repo, branch string) func(*testing.T) { + return func(t *testing.T) { + csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/branches", url.PathEscape(owner), url.PathEscape(repo))) + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/branches/delete?name=%s", url.PathEscape(owner), url.PathEscape(repo), url.QueryEscape(branch)), map[string]string{ + "_csrf": csrf, + }) + ctx.Session.MakeRequest(t, req, http.StatusOK) + } +} diff --git a/services/pull/patch.go b/services/pull/patch.go index 96145fcd92..776bfdb751 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -21,30 +21,17 @@ import ( // DownloadDiffOrPatch will write the patch for the pr to the writer func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error { - // Clone base repo. - tmpBasePath, err := createTemporaryRepo(pr) - if err != nil { - log.Error("CreateTemporaryPath: %v", err) + if err := pr.LoadBaseRepo(); err != nil { + log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index, pr.ID) return err } - defer func() { - if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { - log.Error("DownloadDiff: RemoveTemporaryPath: %s", err) - } - }() - gitRepo, err := git.OpenRepository(tmpBasePath) + gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) if err != nil { return fmt.Errorf("OpenRepository: %v", err) } defer gitRepo.Close() - - pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath) - if err != nil { - pr.MergeBase = "base" - } - pr.MergeBase = strings.TrimSpace(pr.MergeBase) - if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil { + if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil { log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) }