Skip to content

Commit 791339a

Browse files
lunnylunny
committed
Fix bug
1 parent 96f6060 commit 791339a

File tree

5 files changed

+58
-87
lines changed

5 files changed

+58
-87
lines changed

‎routers/web/repo/pull.go‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -653,46 +653,51 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) {
653653
return
654654
}
655655

656-
ctx.Data["IsShowingOnlySingleCommit"] = beforeCommitID != "" && beforeCommitID == afterCommitID
656+
isSingleCommit := beforeCommitID == "" && afterCommitID != ""
657+
ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit
657658
isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID)
658659
ctx.Data["IsShowingAllCommits"] = isShowAllCommits
659660

660-
if beforeCommitID == "" {
661-
beforeCommitID = prInfo.MergeBase
661+
var beforeCommit, afterCommit *git.Commit
662+
if !isSingleCommit {
663+
if beforeCommitID == "" || beforeCommitID == prInfo.MergeBase {
664+
beforeCommitID = prInfo.MergeBase
665+
// mergebase commit is not in the list of the pull request commits
666+
beforeCommit, err = gitRepo.GetCommit(beforeCommitID)
667+
if err != nil {
668+
ctx.ServerError("GetCommit", err)
669+
return
670+
}
671+
} else {
672+
beforeCommit = indexCommit(prInfo.Commits, beforeCommitID)
673+
if beforeCommit == nil {
674+
ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits")
675+
return
676+
}
677+
}
662678
}
663-
if afterCommitID == "" {
679+
680+
if afterCommitID == "" || afterCommitID == headCommitID {
664681
afterCommitID = headCommitID
665682
}
683+
afterCommit = indexCommit(prInfo.Commits, afterCommitID)
684+
if afterCommit == nil {
685+
ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits")
686+
return
687+
}
666688

667-
var beforeCommit, afterCommit *git.Commit
668-
if beforeCommitID != prInfo.MergeBase {
669-
beforeCommit = indexCommit(prInfo.Commits, beforeCommitID)
670-
if beforeCommit == nil {
671-
ctx.NotFound(errors.New("before commit not found in PR commits"))
672-
return
673-
}
674-
beforeCommit, err = beforeCommit.Parent(0)
689+
if isSingleCommit {
690+
beforeCommit, err = afterCommit.Parent(0)
675691
if err != nil {
676692
ctx.ServerError("GetParentCommit", err)
677693
return
678694
}
679695
beforeCommitID = beforeCommit.ID.String()
680-
} else { // mergebase commit is not in the list of the pull request commits
681-
beforeCommit, err = gitRepo.GetCommit(beforeCommitID)
682-
if err != nil {
683-
ctx.ServerError("GetCommit", err)
684-
return
685-
}
686-
}
687-
688-
afterCommit = indexCommit(prInfo.Commits, afterCommitID)
689-
if afterCommit == nil {
690-
ctx.NotFound(errors.New("after commit not found in PR commits"))
691-
return
692696
}
693697

694698
ctx.Data["Username"] = ctx.Repo.Owner.Name
695699
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
700+
ctx.Data["MergeBase"] = prInfo.MergeBase
696701
ctx.Data["AfterCommitID"] = afterCommitID
697702
ctx.Data["BeforeCommitID"] = beforeCommitID
698703

@@ -883,7 +888,9 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) {
883888
}
884889

885890
func ViewPullFilesForSingleCommit(ctx *context.Context) {
886-
viewPullFiles(ctx, ctx.PathParam("sha"), ctx.PathParam("sha"))
891+
// it doesn't support showing files from mergebase to the special commit
892+
// otherwise it will be ambiguous
893+
viewPullFiles(ctx, "", ctx.PathParam("sha"))
887894
}
888895

889896
func ViewPullFilesForRange(ctx *context.Context) {

‎routers/web/repo/pull_review.go‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,6 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
178178
ctx.ServerError("GetRefCommitID", err)
179179
return
180180
}
181-
182181
prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID)
183182
if err != nil {
184183
ctx.ServerError("CommitIDsBetween", err)
@@ -187,27 +186,12 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
187186

188187
if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase {
189188
beforeCommitID = comment.Issue.PullRequest.MergeBase
190-
} else {
191-
// beforeCommitID must be one of the pull request commits
192-
if !slices.Contains(prCommitIDs, beforeCommitID) {
193-
ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID))
194-
return
195-
}
196-
197-
beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID)
198-
if err != nil {
199-
ctx.ServerError("GetCommit", err)
200-
return
201-
}
202-
203-
beforeCommit, err = beforeCommit.Parent(0)
204-
if err != nil {
205-
ctx.ServerError("GetParent", err)
206-
return
207-
}
208-
beforeCommitID = beforeCommit.ID.String()
189+
} else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits
190+
ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID))
191+
return
209192
}
210-
if afterCommitID == "" {
193+
194+
if afterCommitID == "" || afterCommitID == headCommitID {
211195
afterCommitID = headCommitID
212196
} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
213197
ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID))

‎services/pull/review.go‎

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -121,32 +121,18 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
121121
if err != nil {
122122
return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err)
123123
}
124-
125124
prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID)
126125
if err != nil {
127126
return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err)
128127
}
129128

130129
if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase {
131130
beforeCommitID = issue.PullRequest.MergeBase
132-
} else {
133-
// beforeCommitID must be one of the pull request commits
134-
if !slices.Contains(prCommitIDs, beforeCommitID) {
135-
return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)
136-
}
137-
138-
beforeCommit, err := gitRepo.GetCommit(beforeCommitID)
139-
if err != nil {
140-
return nil, fmt.Errorf("GetCommit[%s]: %w", beforeCommitID, err)
141-
}
142-
143-
beforeCommit, err = beforeCommit.Parent(0)
144-
if err != nil {
145-
return nil, fmt.Errorf("GetParent[%s]: %w", beforeCommitID, err)
146-
}
147-
beforeCommitID = beforeCommit.ID.String()
131+
} else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits
132+
return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)
148133
}
149-
if afterCommitID == "" {
134+
135+
if afterCommitID == "" || afterCommitID == headCommitID {
150136
afterCommitID = headCommitID
151137
} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
152138
return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
@@ -648,14 +634,7 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m
648634
}
649635
comment.Line = ReCalculateLineNumber(hunks, comment.Line)
650636
if comment.Line != 0 {
651-
dstCommit, err := gitRepo.GetCommit(dstCommitID)
652-
if err != nil {
653-
return fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err)
654-
}
655-
// If the comment is not the first one or the comment created before the current commit
656-
if len(lineComments[comment.Line]) > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) {
657-
lineComments[comment.Line] = append(lineComments[comment.Line], comment)
658-
}
637+
lineComments[comment.Line] = append(lineComments[comment.Line], comment)
659638
}
660639
}
661640

‎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"}}

‎web_src/js/components/DiffCommitSelector.vue‎

Lines changed: 15 additions & 14 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+
merge_base: el.getAttribute('data-merge-base'),
3536
commits: [] as Array<Commit>,
3637
hoverActivated: false,
3738
lastReviewCommitSha: '',
@@ -179,28 +180,28 @@ export default defineComponent({
179180
* When a commit is clicked with shift this enables the range
180181
* selection. Second click (with shift) defines the end of the
181182
* 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
185183
*/
186184
commitClickedShift(commit: Commit) {
187185
this.hoverActivated = !this.hoverActivated;
188186
commit.selected = true;
189187
// Second click -> determine our range and open links accordingly
190188
if (!this.hoverActivated) {
191189
// 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-
}
190+
const firstSelected = this.commits.findIndex((x) => x.selected);
191+
let start: string;
192+
if (firstSelected === 0) {
193+
start = this.merge_base;
194+
} else {
195+
start = this.commits[firstSelected - 1].id;
196+
}
197+
const end = this.commits.findLast((x) => x.selected).id;
198+
if (start === end) {
199+
// if the start and end are the same, we show this single commit
200+
window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`);
201+
} else if (firstSelected === 0 && end === this.commits.at(-1).id) {
202+
// if the first commit is selected and the last commit is selected, we show all commits
203+
window.location.assign(`${this.issueLink}/files${this.queryParams}`);
201204
} else {
202-
const start = this.commits[this.commits.findIndex((x) => x.selected) - 1].id;
203-
const end = this.commits.findLast((x) => x.selected).id;
204205
window.location.assign(`${this.issueLink}/files/${start}..${end}${this.queryParams}`);
205206
}
206207
}

0 commit comments

Comments
 (0)