Fix bug

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 == "" {
+	var beforeCommit, afterCommit *git.Commit
-		beforeCommitID = prInfo.MergeBase
+	if !isSingleCommit {
-	}
+		if beforeCommitID == "" || beforeCommitID == prInfo.MergeBase {
-	if afterCommitID == "" {
+			beforeCommitID = prInfo.MergeBase
-		afterCommitID = headCommitID
+			// 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 afterCommitID == "" || afterCommitID == headCommitID {
-	if beforeCommitID != prInfo.MergeBase {
+		afterCommitID = headCommitID
-		beforeCommit = indexCommit(prInfo.Commits, beforeCommitID)
+	}
-		if beforeCommit == nil {
+	afterCommit = indexCommit(prInfo.Commits, afterCommitID)
-			ctx.NotFound(errors.New("before commit not found in PR commits"))
+	if afterCommit == nil {
-			return
+		ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits")
-		}
+		return
-		beforeCommit, err = beforeCommit.Parent(0)
+	}
+
+	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) {

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 {
+	} else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits
-		// 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))
-		if !slices.Contains(prCommitIDs, beforeCommitID) {
+		return
-			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()
 	}
-	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))

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 {
+	} else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits
-		// beforeCommitID must be one of the pull request commits
+		return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)
-		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()
 	}
-	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)
+					lineComments[comment.Line] = append(lineComments[comment.Line], comment)
-					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)
-					}
 				}
 			}
 

templates/repo/diff/box.tmpl

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

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<string, string>,
+      merge_base: el.getAttribute('data-merge-base'),
       commits: [] as Array<Commit>,
       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) {
+        const firstSelected = this.commits.findIndex((x) => x.selected);
-          // first commit is selected - generate a short url with only target sha
+        let start: string;
-          const lastCommitIdx = this.commits.findLastIndex((x) => x.selected);
+        if (firstSelected === 0) {
-          if (lastCommitIdx === this.commits.length - 1) {
+          start = this.merge_base;
-            // user selected all commits - just show the normal diff page
+        } else {
-            window.location.assign(`${this.issueLink}/files${this.queryParams}`);
+          start = this.commits[firstSelected - 1].id;
-          } else {
+        }
-            window.location.assign(`${this.issueLink}/files/${this.commits[lastCommitIdx].id}${this.queryParams}`);
+        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}`);
         }
       }