From dda0cf8ca8d6f7f39d9c8ff91d30549ff030eca3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 3 Apr 2020 09:14:08 +0100 Subject: [PATCH 1/3] 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 --- services/pull/patch.go | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index 96145fcd92d6..776bfdb751a8 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) } From 5a182a397b6de8dfe9cc63dacaee9346722a6d54 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 3 Apr 2020 10:44:33 +0100 Subject: [PATCH 2/3] Add tests to ensure that diff does not change Signed-off-by: Andrew Thornton --- integrations/git_test.go | 44 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 8e02bc4db904..a52cc4c5100f 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,48 @@ 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("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()) + }) } } From 7f9af9eff9624653054bbc9bc55bf2776f9fefcc Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 3 Apr 2020 11:32:30 +0100 Subject: [PATCH 3/3] Ensure diffs and pulls pages work if head branch is deleted too Signed-off-by: Andrew Thornton --- integrations/git_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/integrations/git_test.go b/integrations/git_test.go index a52cc4c5100f..8813f34be78f 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -445,6 +445,20 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun 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)) @@ -531,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) + } +}