From 9c8d55daf88a02d5e66225542cda2745422f77f7 Mon Sep 17 00:00:00 2001 From: Elisei Roca Date: Tue, 19 May 2026 20:06:29 +0200 Subject: [PATCH] fix(pull): handle empty pull request files view to allow reviews (#37783) Co-authored-by: wxiaoguang --- routers/web/repo/pull.go | 59 ++++++++++++++++----------- tests/integration/pull_status_test.go | 31 +++++++++----- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4c1a2afb757..ff20c14e440 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -683,6 +683,8 @@ func indexCommit(commits []*git.Commit, commitID string) *git.Commit { // ViewPullFiles render pull request changed files list page func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { + var err error + ctx.Data["PageIsPullList"] = true ctx.Data["PageIsPullFiles"] = true @@ -707,44 +709,53 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { headCommitID := prCompareInfo.HeadCommitID isSingleCommit := beforeCommitID == "" && afterCommitID != "" - ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit + // FIXME: when afterCommitID==headCommitID, isSingleCommit and isShowAllCommits can be both true, which doesn't seem right isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prCompareInfo.CompareBase) && (afterCommitID == "" || afterCommitID == headCommitID) + + ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit ctx.Data["IsShowingAllCommits"] = isShowAllCommits - if afterCommitID == "" || afterCommitID == headCommitID { - afterCommitID = headCommitID - } + // "commits list" is half-open, half-closed: (base, head] + // * base commit is not in the list + // * if the PR is empty, the list is also empty (head commit is not in the list) + + afterCommitID = util.IfZero(afterCommitID, headCommitID) afterCommit := indexCommit(prCompareInfo.Commits, afterCommitID) + if afterCommit == nil && afterCommitID == headCommitID { + afterCommit, err = gitRepo.GetCommit(afterCommitID) + if err != nil { + ctx.ServerError("GetCommit(afterCommitID)", err) + return + } + } if afterCommit == nil { - ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits") + ctx.NotFound(nil) return } var beforeCommit *git.Commit - var err error - if !isSingleCommit { - if beforeCommitID == "" || beforeCommitID == prCompareInfo.CompareBase { - beforeCommitID = prCompareInfo.CompareBase - // merge base commit is not in the list of the pull request commits - beforeCommit, err = gitRepo.GetCommit(beforeCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - } else { - beforeCommit = indexCommit(prCompareInfo.Commits, beforeCommitID) - if beforeCommit == nil { - ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits") - return - } - } - } else { + if isSingleCommit { beforeCommit, err = afterCommit.Parent(0) if err != nil { - ctx.ServerError("Parent", err) + ctx.ServerError("afterCommit.Parent", err) return } beforeCommitID = beforeCommit.ID.String() + } else { + beforeCommitID = util.IfZero(beforeCommitID, prCompareInfo.CompareBase) + beforeCommit = indexCommit(prCompareInfo.Commits, beforeCommitID) + if beforeCommit == nil && beforeCommitID == prCompareInfo.CompareBase { + // base commit is not in the list of the pull request commits + beforeCommit, err = gitRepo.GetCommit(beforeCommitID) + if err != nil { + ctx.ServerError("GetCommit(beforeCommitID)", err) + return + } + } + } + if beforeCommit == nil { + ctx.NotFound(nil) + return } ctx.Data["CompareInfo"] = prCompareInfo diff --git a/tests/integration/pull_status_test.go b/tests/integration/pull_status_test.go index 16b415c364b..54ceb3b39c5 100644 --- a/tests/integration/pull_status_test.go +++ b/tests/integration/pull_status_test.go @@ -140,20 +140,29 @@ func TestPullCreate_EmptyChangesWithDifferentCommits(t *testing.T) { func TestPullCreate_EmptyChangesWithSameCommits(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { session := loginUser(t, "user1") - testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") - testCreateBranch(t, session, "user1", "repo1", "branch/master", "status1", http.StatusSeeOther) - req := NewRequestWithValues(t, "POST", "/user1/repo1/compare/master...status1", - map[string]string{ - "title": "pull request from status1", - }, - ) - session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", "/user1/repo1/pulls/1") - resp := session.MakeRequest(t, req, http.StatusOK) - doc := NewHTMLParser(t, resp.Body) + testCreateBranch(t, session, "user2", "repo1", "branch/master", "empty-pr-branch", http.StatusSeeOther) + resp := testPullCreateDirectly(t, session, createPullRequestOptions{ + BaseRepoOwner: "user2", + BaseRepoName: "repo1", + BaseBranch: "master", + HeadBranch: "empty-pr-branch", + Title: "empty pr test", + }) + prURL := test.RedirectURL(resp) + + // check the "merge box" text + req := NewRequest(t, "GET", prURL) + resp = session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) text := strings.TrimSpace(doc.doc.Find(".merge-section").Text()) assert.Contains(t, text, "This branch is already included in the target branch. There is nothing to merge.") + + // check the "files" tab content + req = NewRequest(t, "GET", prURL+"/files") + resp = session.MakeRequest(t, req, http.StatusOK) + doc = NewHTMLParser(t, resp.Body) + assert.Equal(t, "Diff Content Not Available", strings.TrimSpace(doc.Find("#diff-container").Text())) }) }