Skip to content

Commit be2a6b4

Browse files
lunnylunny
authored
Fix bug when review pull request commits (#35192)
The commit range in the UI follows a half-open, half-closed convention: (,]. When reviewing a range of commits, the beforeCommitID should be set to the commit immediately preceding the first selected commit. For single-commit reviews, we must identify and use the previous commit of that specific commit. The endpoint ViewPullFilesStartingFromCommit is currently unused and can be safely removed. Fix #35157 Replace #35184 Partially extract from #35077
1 parent 1f676b3 commit be2a6b4

File tree

5 files changed

+89
-108
lines changed

5 files changed

+89
-108
lines changed

‎routers/web/repo/pull.go‎

Lines changed: 61 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -643,8 +643,17 @@ func ViewPullCommits(ctx *context.Context) {
643643
ctx.HTML(http.StatusOK, tplPullCommits)
644644
}
645645

646+
func indexCommit(commits []*git.Commit, commitID string) *git.Commit {
647+
for i := range commits {
648+
if commits[i].ID.String() == commitID {
649+
return commits[i]
650+
}
651+
}
652+
return nil
653+
}
654+
646655
// ViewPullFiles render pull request changed files list page
647-
func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommit string, willShowSpecifiedCommitRange, willShowSpecifiedCommit bool) {
656+
func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) {
648657
ctx.Data["PageIsPullList"] = true
649658
ctx.Data["PageIsPullFiles"] = true
650659

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

657-
var (
658-
startCommitID string
659-
endCommitID string
660-
gitRepo = ctx.Repo.GitRepo
661-
)
666+
gitRepo := ctx.Repo.GitRepo
662667

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

671-
// Validate the given commit sha to show (if any passed)
672-
if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
673-
foundStartCommit := len(specifiedStartCommit) == 0
674-
foundEndCommit := len(specifiedEndCommit) == 0
675-
676-
if !(foundStartCommit && foundEndCommit) {
677-
for _, commit := range prInfo.Commits {
678-
if commit.ID.String() == specifiedStartCommit {
679-
foundStartCommit = true
680-
}
681-
if commit.ID.String() == specifiedEndCommit {
682-
foundEndCommit = true
683-
}
684-
685-
if foundStartCommit && foundEndCommit {
686-
break
687-
}
688-
}
689-
}
690-
691-
if !(foundStartCommit && foundEndCommit) {
692-
ctx.NotFound(nil)
693-
return
694-
}
695-
}
696-
697-
if ctx.Written() {
698-
return
699-
}
700-
701676
headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
702677
if err != nil {
703678
ctx.ServerError("GetRefCommitID", err)
704679
return
705680
}
706681

707-
ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit
682+
isSingleCommit := beforeCommitID == "" && afterCommitID != ""
683+
ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit
684+
isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID)
685+
ctx.Data["IsShowingAllCommits"] = isShowAllCommits
708686

709-
if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
710-
if len(specifiedEndCommit) > 0 {
711-
endCommitID = specifiedEndCommit
712-
} else {
713-
endCommitID = headCommitID
714-
}
715-
if len(specifiedStartCommit) > 0 {
716-
startCommitID = specifiedStartCommit
687+
if afterCommitID == "" || afterCommitID == headCommitID {
688+
afterCommitID = headCommitID
689+
}
690+
afterCommit := indexCommit(prInfo.Commits, afterCommitID)
691+
if afterCommit == nil {
692+
ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits")
693+
return
694+
}
695+
696+
var beforeCommit *git.Commit
697+
if !isSingleCommit {
698+
if beforeCommitID == "" || beforeCommitID == prInfo.MergeBase {
699+
beforeCommitID = prInfo.MergeBase
700+
// mergebase commit is not in the list of the pull request commits
701+
beforeCommit, err = gitRepo.GetCommit(beforeCommitID)
702+
if err != nil {
703+
ctx.ServerError("GetCommit", err)
704+
return
705+
}
717706
} else {
718-
startCommitID = prInfo.MergeBase
707+
beforeCommit = indexCommit(prInfo.Commits, beforeCommitID)
708+
if beforeCommit == nil {
709+
ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits")
710+
return
711+
}
719712
}
720-
ctx.Data["IsShowingAllCommits"] = false
721713
} else {
722-
endCommitID = headCommitID
723-
startCommitID = prInfo.MergeBase
724-
ctx.Data["IsShowingAllCommits"] = true
714+
beforeCommit, err = afterCommit.Parent(0)
715+
if err != nil {
716+
ctx.ServerError("Parent", err)
717+
return
718+
}
719+
beforeCommitID = beforeCommit.ID.String()
725720
}
726721

727722
ctx.Data["Username"] = ctx.Repo.Owner.Name
728723
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
729-
ctx.Data["AfterCommitID"] = endCommitID
730-
ctx.Data["BeforeCommitID"] = startCommitID
731-
732-
fileOnly := ctx.FormBool("file-only")
724+
ctx.Data["MergeBase"] = prInfo.MergeBase
725+
ctx.Data["AfterCommitID"] = afterCommitID
726+
ctx.Data["BeforeCommitID"] = beforeCommitID
733727

734728
maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles
735729
files := ctx.FormStrings("files")
730+
fileOnly := ctx.FormBool("file-only")
736731
if fileOnly && (len(files) == 2 || len(files) == 1) {
737732
maxLines, maxFiles = -1, -1
738733
}
739734

740735
diffOptions := &gitdiff.DiffOptions{
741-
AfterCommitID: endCommitID,
736+
BeforeCommitID: beforeCommitID,
737+
AfterCommitID: afterCommitID,
742738
SkipTo: ctx.FormString("skip-to"),
743739
MaxLines: maxLines,
744740
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
745741
MaxFiles: maxFiles,
746742
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
747743
}
748744

749-
if !willShowSpecifiedCommit {
750-
diffOptions.BeforeCommitID = startCommitID
751-
}
752-
753745
diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, diffOptions, files...)
754746
if err != nil {
755747
ctx.ServerError("GetDiff", err)
@@ -761,15 +753,15 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
761753
// as the viewed information is designed to be loaded only on latest PR
762754
// diff and if you're signed in.
763755
var reviewState *pull_model.ReviewState
764-
if ctx.IsSigned && !willShowSpecifiedCommit && !willShowSpecifiedCommitRange {
756+
if ctx.IsSigned && isShowAllCommits {
765757
reviewState, err = gitdiff.SyncUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diff, diffOptions)
766758
if err != nil {
767759
ctx.ServerError("SyncUserSpecificDiff", err)
768760
return
769761
}
770762
}
771763

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

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

839-
baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID)
840-
if err != nil {
841-
ctx.ServerError("GetCommit", err)
842-
return
843-
}
844-
commit, err := gitRepo.GetCommit(endCommitID)
845-
if err != nil {
846-
ctx.ServerError("GetCommit", err)
847-
return
848-
}
849-
850831
if ctx.IsSigned && ctx.Doer != nil {
851832
if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil {
852833
ctx.ServerError("CanMarkConversation", err)
853834
return
854835
}
855836
}
856837

857-
setCompareContext(ctx, baseCommit, commit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
838+
setCompareContext(ctx, beforeCommit, afterCommit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
858839

859840
assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository)
860841
if err != nil {
@@ -901,7 +882,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
901882
ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool {
902883
return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee)
903884
}
904-
if !willShowSpecifiedCommit && !willShowSpecifiedCommitRange && pull.Flow == issues_model.PullRequestFlowGithub {
885+
if isShowAllCommits && pull.Flow == issues_model.PullRequestFlowGithub {
905886
if err := pull.LoadHeadRepo(ctx); err != nil {
906887
ctx.ServerError("LoadHeadRepo", err)
907888
return
@@ -930,19 +911,17 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
930911
}
931912

932913
func ViewPullFilesForSingleCommit(ctx *context.Context) {
933-
viewPullFiles(ctx, "", ctx.PathParam("sha"), true, true)
914+
// it doesn't support showing files from mergebase to the special commit
915+
// otherwise it will be ambiguous
916+
viewPullFiles(ctx, "", ctx.PathParam("sha"))
934917
}
935918

936919
func ViewPullFilesForRange(ctx *context.Context) {
937-
viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"), true, false)
938-
}
939-
940-
func ViewPullFilesStartingFromCommit(ctx *context.Context) {
941-
viewPullFiles(ctx, "", ctx.PathParam("sha"), true, false)
920+
viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"))
942921
}
943922

944923
func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) {
945-
viewPullFiles(ctx, "", "", false, false)
924+
viewPullFiles(ctx, "", "")
946925
}
947926

948927
// UpdatePullRequest merge PR's baseBranch into headBranch

‎routers/web/web.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1531,7 +1531,7 @@ func registerWebRoutes(m *web.Router) {
15311531
m.Group("/commits", func() {
15321532
m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits)
15331533
m.Get("/list", repo.GetPullCommits)
1534-
m.Get("/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
1534+
m.Get("/{sha:[a-f0-9]{7,64}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
15351535
})
15361536
m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), repo.MergePullRequest)
15371537
m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest)
@@ -1540,8 +1540,7 @@ func registerWebRoutes(m *web.Router) {
15401540
m.Post("/cleanup", context.RepoMustNotBeArchived(), repo.CleanUpPullRequest)
15411541
m.Group("/files", func() {
15421542
m.Get("", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr)
1543-
m.Get("/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit)
1544-
m.Get("/{shaFrom:[a-f0-9]{7,40}}..{shaTo:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange)
1543+
m.Get("/{shaFrom:[a-f0-9]{7,64}}..{shaTo:[a-f0-9]{7,64}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange)
15451544
m.Group("/reviews", func() {
15461545
m.Get("/new_comment", repo.RenderNewCodeCommentForm)
15471546
m.Post("/comments", web.Bind(forms.CodeCommentForm{}), repo.SetShowOutdatedComments, repo.CreateCodeComment)

‎templates/repo/diff/box.tmpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
{{template "repo/diff/whitespace_dropdown" .}}
3636
{{template "repo/diff/options_dropdown" .}}
3737
{{if .PageIsPullFiles}}
38-
<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"}}">
38+
<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"}}">
3939
{{/* the following will be replaced by vue component, but this avoids any loading artifacts till the vue component is initialized */}}
4040
<div class="ui jump dropdown tiny basic button custom">
4141
{{svg "octicon-git-commit"}}

‎tests/integration/pull_diff_test.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,6 @@ func TestPullDiff_CommitRangePRDiff(t *testing.T) {
2525
doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"})
2626
}
2727

28-
func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) {
29-
doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"})
30-
}
31-
3228
func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) {
3329
defer tests.PrepareTestEnv(t)()
3430

‎web_src/js/components/DiffCommitSelector.vue‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ export default defineComponent({
3232
locale: {
3333
filter_changes_by_commit: el.getAttribute('data-filter_changes_by_commit'),
3434
} as Record<string, string>,
35+
mergeBase: el.getAttribute('data-merge-base'),
3536
commits: [] as Array<Commit>,
3637
hoverActivated: false,
3738
lastReviewCommitSha: '',
@@ -176,32 +177,38 @@ export default defineComponent({
176177
}
177178
},
178179
/**
179-
* When a commit is clicked with shift this enables the range
180-
* selection. Second click (with shift) defines the end of the
181-
* range. This opens the diff of this range
182-
* Exception: first commit is the first commit of this PR. Then
183-
* the diff from beginning of PR up to the second clicked commit is
184-
* opened
180+
* When a commit is clicked while holding Shift, it enables range selection.
181+
* - The range selection is a half-open, half-closed range, meaning it excludes the start commit but includes the end commit.
182+
* - The start of the commit range is always the previous commit of the first clicked commit.
183+
* - If the first commit in the list is clicked, the mergeBase will be used as the start of the range instead.
184+
* - The second Shift-click defines the end of the range.
185+
* - Once both are selected, the diff view for the selected commit range will open.
185186
*/
186187
commitClickedShift(commit: Commit) {
187188
this.hoverActivated = !this.hoverActivated;
188189
commit.selected = true;
189190
// Second click -> determine our range and open links accordingly
190191
if (!this.hoverActivated) {
192+
// since at least one commit is selected, we can determine the range
191193
// find all selected commits and generate a link
192-
if (this.commits[0].selected) {
193-
// first commit is selected - generate a short url with only target sha
194-
const lastCommitIdx = this.commits.findLastIndex((x) => x.selected);
195-
if (lastCommitIdx === this.commits.length - 1) {
196-
// user selected all commits - just show the normal diff page
197-
window.location.assign(`${this.issueLink}/files${this.queryParams}`);
198-
} else {
199-
window.location.assign(`${this.issueLink}/files/${this.commits[lastCommitIdx].id}${this.queryParams}`);
200-
}
194+
const firstSelected = this.commits.findIndex((x) => x.selected);
195+
const lastSelected = this.commits.findLastIndex((x) => x.selected);
196+
let beforeCommitID: string;
197+
if (firstSelected === 0) {
198+
beforeCommitID = this.mergeBase;
201199
} else {
202-
const start = this.commits[this.commits.findIndex((x) => x.selected) - 1].id;
203-
const end = this.commits.findLast((x) => x.selected).id;
204-
window.location.assign(`${this.issueLink}/files/${start}..${end}${this.queryParams}`);
200+
beforeCommitID = this.commits[firstSelected - 1].id;
201+
}
202+
const afterCommitID = this.commits[lastSelected].id;
203+
204+
if (firstSelected === lastSelected) {
205+
// if the start and end are the same, we show this single commit
206+
window.location.assign(`${this.issueLink}/commits/${afterCommitID}${this.queryParams}`);
207+
} else if (beforeCommitID === this.mergeBase && afterCommitID === this.commits.at(-1).id) {
208+
// if the first commit is selected and the last commit is selected, we show all commits
209+
window.location.assign(`${this.issueLink}/files${this.queryParams}`);
210+
} else {
211+
window.location.assign(`${this.issueLink}/files/${beforeCommitID}..${afterCommitID}${this.queryParams}`);
205212
}
206213
}
207214
},

0 commit comments

Comments
 (0)