From 99a364a9dc803c15efd3ca2ba2fed3179a6aa37a Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 3 Apr 2020 15:06:54 +0100 Subject: [PATCH] Generate Diff and Patch direct from Pull head (#10936) (#10938) Backport #10936 * Generate Diff and Patch direct from Pull head Fix #10932 Also fix "Empty Diff/Patch File when pull is merged" Closes #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 --- integrations/git_test.go | 69 ++++++++++++++++++++++++++++++++++++++-- services/pull/patch.go | 25 ++++----------- 2 files changed, 72 insertions(+), 22 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 57a2997b36..295d889248 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -31,32 +31,19 @@ func DownloadPatch(pr *models.PullRequest, w io.Writer, patch bool) error { // 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 { - 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) - 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) + 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) } return nil }