Skip to content

Fix bug when review pull request commits (#35192) #35246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2025
+89 −108
Merged

Fix bug when review pull request commits (#35192) #35246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 61 additions & 82 deletions routers/web/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,8 +643,17 @@ func ViewPullCommits(ctx *context.Context) {
ctx.HTML(http.StatusOK, tplPullCommits)
}

func indexCommit(commits []*git.Commit, commitID string) *git.Commit {
for i := range commits {
if commits[i].ID.String() == commitID {
return commits[i]
}
}
return nil
}

// ViewPullFiles render pull request changed files list page
func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommit string, willShowSpecifiedCommitRange, willShowSpecifiedCommit bool) {
func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) {
ctx.Data["PageIsPullList"] = true
ctx.Data["PageIsPullFiles"] = true

Expand All @@ -654,11 +663,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
}
pull := issue.PullRequest

var (
startCommitID string
endCommitID string
gitRepo = ctx.Repo.GitRepo
)
gitRepo := ctx.Repo.GitRepo

prInfo := preparePullViewPullInfo(ctx, issue)
if ctx.Written() {
Expand All @@ -668,88 +673,75 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
return
}

// Validate the given commit sha to show (if any passed)
if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
foundStartCommit := len(specifiedStartCommit) == 0
foundEndCommit := len(specifiedEndCommit) == 0

if !(foundStartCommit && foundEndCommit) {
for _, commit := range prInfo.Commits {
if commit.ID.String() == specifiedStartCommit {
foundStartCommit = true
}
if commit.ID.String() == specifiedEndCommit {
foundEndCommit = true
}

if foundStartCommit && foundEndCommit {
break
}
}
}

if !(foundStartCommit && foundEndCommit) {
ctx.NotFound(nil)
return
}
}

if ctx.Written() {
return
}

headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName())
if err != nil {
ctx.ServerError("GetRefCommitID", err)
return
}

ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit
isSingleCommit := beforeCommitID == "" && afterCommitID != ""
ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit
isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID)
ctx.Data["IsShowingAllCommits"] = isShowAllCommits

if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
if len(specifiedEndCommit) > 0 {
endCommitID = specifiedEndCommit
} else {
endCommitID = headCommitID
}
if len(specifiedStartCommit) > 0 {
startCommitID = specifiedStartCommit
if afterCommitID == "" || afterCommitID == headCommitID {
afterCommitID = headCommitID
}
afterCommit := indexCommit(prInfo.Commits, afterCommitID)
if afterCommit == nil {
ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits")
return
}

var beforeCommit *git.Commit
if !isSingleCommit {
if beforeCommitID == "" || beforeCommitID == prInfo.MergeBase {
beforeCommitID = prInfo.MergeBase
// mergebase 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 {
startCommitID = prInfo.MergeBase
beforeCommit = indexCommit(prInfo.Commits, beforeCommitID)
if beforeCommit == nil {
ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits")
return
}
}
ctx.Data["IsShowingAllCommits"] = false
} else {
endCommitID = headCommitID
startCommitID = prInfo.MergeBase
ctx.Data["IsShowingAllCommits"] = true
beforeCommit, err = afterCommit.Parent(0)
if err != nil {
ctx.ServerError("Parent", err)
return
}
beforeCommitID = beforeCommit.ID.String()
}

ctx.Data["Username"] = ctx.Repo.Owner.Name
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
ctx.Data["AfterCommitID"] = endCommitID
ctx.Data["BeforeCommitID"] = startCommitID

fileOnly := ctx.FormBool("file-only")
ctx.Data["MergeBase"] = prInfo.MergeBase
ctx.Data["AfterCommitID"] = afterCommitID
ctx.Data["BeforeCommitID"] = beforeCommitID

maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles
files := ctx.FormStrings("files")
fileOnly := ctx.FormBool("file-only")
if fileOnly && (len(files) == 2 || len(files) == 1) {
maxLines, maxFiles = -1, -1
}

diffOptions := &gitdiff.DiffOptions{
AfterCommitID: endCommitID,
BeforeCommitID: beforeCommitID,
AfterCommitID: afterCommitID,
SkipTo: ctx.FormString("skip-to"),
MaxLines: maxLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
}

if !willShowSpecifiedCommit {
diffOptions.BeforeCommitID = startCommitID
}

diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, diffOptions, files...)
if err != nil {
ctx.ServerError("GetDiff", err)
Expand All @@ -761,15 +753,15 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
// as the viewed information is designed to be loaded only on latest PR
// diff and if you're signed in.
var reviewState *pull_model.ReviewState
if ctx.IsSigned && !willShowSpecifiedCommit && !willShowSpecifiedCommitRange {
if ctx.IsSigned && isShowAllCommits {
reviewState, err = gitdiff.SyncUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diff, diffOptions)
if err != nil {
ctx.ServerError("SyncUserSpecificDiff", err)
return
}
}

diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, startCommitID, endCommitID)
diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, beforeCommitID, afterCommitID)
if err != nil {
ctx.ServerError("GetDiffShortStat", err)
return
Expand Down Expand Up @@ -816,7 +808,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi

if !fileOnly {
// note: use mergeBase is set to false because we already have the merge base from the pull request info
diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, startCommitID, endCommitID)
diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, beforeCommitID, afterCommitID)
if err != nil {
ctx.ServerError("GetDiffTree", err)
return
Expand All @@ -836,25 +828,14 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
ctx.Data["Diff"] = diff
ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0

baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID)
if err != nil {
ctx.ServerError("GetCommit", err)
return
}
commit, err := gitRepo.GetCommit(endCommitID)
if err != nil {
ctx.ServerError("GetCommit", err)
return
}

if ctx.IsSigned && ctx.Doer != nil {
if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil {
ctx.ServerError("CanMarkConversation", err)
return
}
}

setCompareContext(ctx, baseCommit, commit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
setCompareContext(ctx, beforeCommit, afterCommit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)

assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository)
if err != nil {
Expand Down Expand Up @@ -901,7 +882,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool {
return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee)
}
if !willShowSpecifiedCommit && !willShowSpecifiedCommitRange && pull.Flow == issues_model.PullRequestFlowGithub {
if isShowAllCommits && pull.Flow == issues_model.PullRequestFlowGithub {
if err := pull.LoadHeadRepo(ctx); err != nil {
ctx.ServerError("LoadHeadRepo", err)
return
Expand Down Expand Up @@ -930,19 +911,17 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
}

func ViewPullFilesForSingleCommit(ctx *context.Context) {
viewPullFiles(ctx, "", ctx.PathParam("sha"), true, true)
// it doesn't support showing files from mergebase to the special commit
// otherwise it will be ambiguous
viewPullFiles(ctx, "", ctx.PathParam("sha"))
}

func ViewPullFilesForRange(ctx *context.Context) {
viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"), true, false)
}

func ViewPullFilesStartingFromCommit(ctx *context.Context) {
viewPullFiles(ctx, "", ctx.PathParam("sha"), true, false)
viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"))
}

func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) {
viewPullFiles(ctx, "", "", false, false)
viewPullFiles(ctx, "", "")
}

// UpdatePullRequest merge PR's baseBranch into headBranch
Expand Down
5 changes: 2 additions & 3 deletions routers/web/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1509,7 +1509,7 @@ func registerWebRoutes(m *web.Router) {
m.Group("/commits", func() {
m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits)
m.Get("/list", repo.GetPullCommits)
m.Get("/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
m.Get("/{sha:[a-f0-9]{7,64}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
})
m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), repo.MergePullRequest)
m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest)
Expand All @@ -1518,8 +1518,7 @@ func registerWebRoutes(m *web.Router) {
m.Post("/cleanup", context.RepoMustNotBeArchived(), repo.CleanUpPullRequest)
m.Group("/files", func() {
m.Get("", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr)
m.Get("/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit)
m.Get("/{shaFrom:[a-f0-9]{7,40}}..{shaTo:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange)
m.Get("/{shaFrom:[a-f0-9]{7,64}}..{shaTo:[a-f0-9]{7,64}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange)
m.Group("/reviews", func() {
m.Get("/new_comment", repo.RenderNewCodeCommentForm)
m.Post("/comments", web.Bind(forms.CodeCommentForm{}), repo.SetShowOutdatedComments, repo.CreateCodeComment)
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/diff/box.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
{{template "repo/diff/whitespace_dropdown" .}}
{{template "repo/diff/options_dropdown" .}}
{{if .PageIsPullFiles}}
<div id="diff-commit-select" data-issuelink="{{$.Issue.Link}}" data-queryparams="?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}" data-filter_changes_by_commit="{{ctx.Locale.Tr "repo.pulls.filter_changes_by_commit"}}">
<div id="diff-commit-select" data-merge-base="{{.MergeBase}}" data-issuelink="{{$.Issue.Link}}" data-queryparams="?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}" data-filter_changes_by_commit="{{ctx.Locale.Tr "repo.pulls.filter_changes_by_commit"}}">
{{/* the following will be replaced by vue component, but this avoids any loading artifacts till the vue component is initialized */}}
<div class="ui jump dropdown tiny basic button custom">
{{svg "octicon-git-commit"}}
Expand Down
4 changes: 0 additions & 4 deletions tests/integration/pull_diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ func TestPullDiff_CommitRangePRDiff(t *testing.T) {
doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"})
}

func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) {
doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"})
}

func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) {
defer tests.PrepareTestEnv(t)()

Expand Down
43 changes: 25 additions & 18 deletions web_src/js/components/DiffCommitSelector.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default defineComponent({
locale: {
filter_changes_by_commit: el.getAttribute('data-filter_changes_by_commit'),
} as Record<string, string>,
mergeBase: el.getAttribute('data-merge-base'),
commits: [] as Array<Commit>,
hoverActivated: false,
lastReviewCommitSha: '',
Expand Down Expand Up @@ -176,32 +177,38 @@ export default defineComponent({
}
},
/**
* When a commit is clicked with shift this enables the range
* selection. Second click (with shift) defines the end of the
* range. This opens the diff of this range
* Exception: first commit is the first commit of this PR. Then
* the diff from beginning of PR up to the second clicked commit is
* opened
* When a commit is clicked while holding Shift, it enables range selection.
* - The range selection is a half-open, half-closed range, meaning it excludes the start commit but includes the end commit.
* - The start of the commit range is always the previous commit of the first clicked commit.
* - If the first commit in the list is clicked, the mergeBase will be used as the start of the range instead.
* - The second Shift-click defines the end of the range.
* - Once both are selected, the diff view for the selected commit range will open.
*/
commitClickedShift(commit: Commit) {
this.hoverActivated = !this.hoverActivated;
commit.selected = true;
// Second click -> determine our range and open links accordingly
if (!this.hoverActivated) {
// since at least one commit is selected, we can determine the range
// find all selected commits and generate a link
if (this.commits[0].selected) {
// first commit is selected - generate a short url with only target sha
const lastCommitIdx = this.commits.findLastIndex((x) => x.selected);
if (lastCommitIdx === this.commits.length - 1) {
// user selected all commits - just show the normal diff page
window.location.assign(`${this.issueLink}/files${this.queryParams}`);
} else {
window.location.assign(`${this.issueLink}/files/${this.commits[lastCommitIdx].id}${this.queryParams}`);
}
const firstSelected = this.commits.findIndex((x) => x.selected);
const lastSelected = this.commits.findLastIndex((x) => x.selected);
let beforeCommitID: string;
if (firstSelected === 0) {
beforeCommitID = this.mergeBase;
} else {
const start = this.commits[this.commits.findIndex((x) => x.selected) - 1].id;
const end = this.commits.findLast((x) => x.selected).id;
window.location.assign(`${this.issueLink}/files/${start}..${end}${this.queryParams}`);
beforeCommitID = this.commits[firstSelected - 1].id;
}
const afterCommitID = this.commits[lastSelected].id;
if (firstSelected === lastSelected) {
// if the start and end are the same, we show this single commit
window.location.assign(`${this.issueLink}/commits/${afterCommitID}${this.queryParams}`);
} else if (beforeCommitID === this.mergeBase && afterCommitID === this.commits.at(-1).id) {
// if the first commit is selected and the last commit is selected, we show all commits
window.location.assign(`${this.issueLink}/files${this.queryParams}`);
} else {
window.location.assign(`${this.issueLink}/files/${beforeCommitID}..${afterCommitID}${this.queryParams}`);
}
}
},
Expand Down