diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 581d97bd4f6..9c11a8453e5 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -653,46 +653,51 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { return } - ctx.Data["IsShowingOnlySingleCommit"] = beforeCommitID != "" && beforeCommitID == afterCommitID + isSingleCommit := beforeCommitID == "" && afterCommitID != "" + ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID) ctx.Data["IsShowingAllCommits"] = isShowAllCommits - if beforeCommitID == "" { - beforeCommitID = prInfo.MergeBase - } - if afterCommitID == "" { - afterCommitID = headCommitID + var beforeCommit, afterCommit *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 { + beforeCommit = indexCommit(prInfo.Commits, beforeCommitID) + if beforeCommit == nil { + ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits") + return + } + } } - var beforeCommit, afterCommit *git.Commit - if beforeCommitID != prInfo.MergeBase { - beforeCommit = indexCommit(prInfo.Commits, beforeCommitID) - if beforeCommit == nil { - ctx.NotFound(errors.New("before commit not found in PR commits")) - return - } - beforeCommit, err = beforeCommit.Parent(0) + 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 + } + + if isSingleCommit { + beforeCommit, err = afterCommit.Parent(0) if err != nil { ctx.ServerError("GetParentCommit", err) return } beforeCommitID = beforeCommit.ID.String() - } else { // 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 - } - } - - afterCommit = indexCommit(prInfo.Commits, afterCommitID) - if afterCommit == nil { - ctx.NotFound(errors.New("after commit not found in PR commits")) - return } ctx.Data["Username"] = ctx.Repo.Owner.Name ctx.Data["Reponame"] = ctx.Repo.Repository.Name + ctx.Data["MergeBase"] = prInfo.MergeBase ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["BeforeCommitID"] = beforeCommitID @@ -883,7 +888,9 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } func ViewPullFilesForSingleCommit(ctx *context.Context) { - viewPullFiles(ctx, ctx.PathParam("sha"), ctx.PathParam("sha")) + // 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) { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 29b436e9fc3..fcb4b88776c 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -178,7 +178,6 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori ctx.ServerError("GetRefCommitID", err) return } - prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID) if err != nil { ctx.ServerError("CommitIDsBetween", err) @@ -187,27 +186,12 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase { beforeCommitID = comment.Issue.PullRequest.MergeBase - } else { - // beforeCommitID must be one of the pull request commits - if !slices.Contains(prCommitIDs, beforeCommitID) { - ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) - return - } - - beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - - beforeCommit, err = beforeCommit.Parent(0) - if err != nil { - ctx.ServerError("GetParent", err) - return - } - beforeCommitID = beforeCommit.ID.String() + } else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) + return } - if afterCommitID == "" { + + if afterCommitID == "" || afterCommitID == headCommitID { afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)) diff --git a/services/pull/review.go b/services/pull/review.go index ee840cdd9c2..d1b5ef86062 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -121,7 +121,6 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if err != nil { return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) } - prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID) if err != nil { return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err) @@ -129,24 +128,11 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase { beforeCommitID = issue.PullRequest.MergeBase - } else { - // beforeCommitID must be one of the pull request commits - if !slices.Contains(prCommitIDs, beforeCommitID) { - return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID) - } - - beforeCommit, err := gitRepo.GetCommit(beforeCommitID) - if err != nil { - return nil, fmt.Errorf("GetCommit[%s]: %w", beforeCommitID, err) - } - - beforeCommit, err = beforeCommit.Parent(0) - if err != nil { - return nil, fmt.Errorf("GetParent[%s]: %w", beforeCommitID, err) - } - beforeCommitID = beforeCommit.ID.String() + } else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits + return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID) } - if afterCommitID == "" { + + if afterCommitID == "" || afterCommitID == headCommitID { afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits 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 } comment.Line = ReCalculateLineNumber(hunks, comment.Line) if comment.Line != 0 { - dstCommit, err := gitRepo.GetCommit(dstCommitID) - if err != nil { - return fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) - } - // If the comment is not the first one or the comment created before the current commit - if len(lineComments[comment.Line]) > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) - } + lineComments[comment.Line] = append(lineComments[comment.Line], comment) } } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index ade1da655a0..53e6053602b 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -35,7 +35,7 @@ {{template "repo/diff/whitespace_dropdown" .}} {{template "repo/diff/options_dropdown" .}} {{if .PageIsPullFiles}} -
+
{{/* the following will be replaced by vue component, but this avoids any loading artifacts till the vue component is initialized */}}
{{svg "octicon-git-commit"}} diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 4b18694bd2d..03d55e896b9 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -32,6 +32,7 @@ export default defineComponent({ locale: { filter_changes_by_commit: el.getAttribute('data-filter_changes_by_commit'), } as Record, + merge_base: el.getAttribute('data-merge-base'), commits: [] as Array, hoverActivated: false, lastReviewCommitSha: '', @@ -179,9 +180,6 @@ 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 */ commitClickedShift(commit: Commit) { this.hoverActivated = !this.hoverActivated; @@ -189,18 +187,21 @@ export default defineComponent({ // Second click -> determine our range and open links accordingly if (!this.hoverActivated) { // 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); + let start: string; + if (firstSelected === 0) { + start = this.merge_base; + } else { + start = this.commits[firstSelected - 1].id; + } + const end = this.commits.findLast((x) => x.selected).id; + if (start === end) { + // if the start and end are the same, we show this single commit + window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); + } else if (firstSelected === 0 && end === 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 { - 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}`); } }