diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index 174345ff5..fa72f9b64 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -304,3 +304,20 @@ created_unix: 946684830 updated_unix: 978307200 is_locked: false + +- + id: 19 + repo_id: 58 + index: 1 + poster_id: 2 + original_author_id: 0 + name: issue for pr + content: content + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684830 + updated_unix: 978307200 + is_locked: false diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml index 165437f03..e5589ac70 100644 --- a/models/fixtures/pull_request.yml +++ b/models/fixtures/pull_request.yml @@ -76,3 +76,16 @@ base_branch: master merge_base: 2a47ca4b614a9f5a has_merged: false + +- + id: 7 + type: 0 # gitea pull request + status: 2 # mergable + issue_id: 19 + index: 1 + head_repo_id: 58 + base_repo_id: 58 + head_branch: branch1 + base_branch: main + merge_base: cbff181af4c9c7fee3cf6c106699e07d9a3f54e6 + has_merged: false diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 5bb974a7d..bb8715a20 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -607,3 +607,33 @@ repo_id: 52 type: 1 created_unix: 946684810 + +- + id: 91 + repo_id: 58 + type: 1 + created_unix: 946684810 + +- + id: 92 + repo_id: 58 + type: 2 + created_unix: 946684810 + +- + id: 93 + repo_id: 58 + type: 3 + created_unix: 946684810 + +- + id: 94 + repo_id: 58 + type: 4 + created_unix: 946684810 + +- + id: 95 + repo_id: 58 + type: 5 + created_unix: 946684810 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 050a9e2d0..15668e6ca 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -1662,3 +1662,34 @@ is_private: false status: 0 num_issues: 0 + +- + id: 58 # org public repo + owner_id: 2 + owner_name: user2 + lower_name: commitsonpr + name: commitsonpr + default_branch: main + num_watches: 0 + num_stars: 0 + num_forks: 0 + num_issues: 0 + num_closed_issues: 0 + num_pulls: 1 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_projects: 0 + num_closed_projects: 0 + is_private: false + is_empty: false + is_archived: false + is_mirror: false + status: 0 + is_fork: false + fork_id: 0 + is_template: false + template_id: 0 + size: 0 + is_fsck_enabled: true + close_issues_via_commit_in_any_branch: false diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 26bb7a9f4..c7c5c024b 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -66,7 +66,7 @@ num_followers: 2 num_following: 1 num_stars: 2 - num_repos: 13 + num_repos: 14 num_teams: 0 num_members: 0 visibility: 0 diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index 7f1eab197..44e59f5cc 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -538,7 +538,7 @@ func TestCountIssues(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) count, err := issues_model.CountIssues(db.DefaultContext, &issues_model.IssuesOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 18, count) + assert.EqualValues(t, 19, count) } func TestIssueLoadAttributes(t *testing.T) { diff --git a/models/issues/review_list.go b/models/issues/review_list.go index c044fe915..9f50d8e09 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -114,7 +114,7 @@ func FindLatestReviews(ctx context.Context, opts FindReviewOptions) (ReviewList, } sess.In("id", builder. - Select("max ( id ) "). + Select("max(id)"). From("review"). Where(cond). GroupBy("reviewer_id")) diff --git a/models/repo/repo_list_test.go b/models/repo/repo_list_test.go index 573281ea0..e013953c6 100644 --- a/models/repo/repo_list_test.go +++ b/models/repo/repo_list_test.go @@ -235,12 +235,12 @@ func TestSearchRepository(t *testing.T) { { name: "AllPublic/PublicRepositoriesOfUserIncludingCollaborative", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: util.OptionalBoolFalse}, - count: 30, + count: 31, }, { name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: util.OptionalBoolFalse}, - count: 35, + count: 36, }, { name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", @@ -255,7 +255,7 @@ func TestSearchRepository(t *testing.T) { { name: "AllPublic/PublicRepositoriesOfOrganization", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: util.OptionalBoolFalse, Template: util.OptionalBoolFalse}, - count: 30, + count: 31, }, { name: "AllTemplates", diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index dc88c422b..cf04830cb 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1662,6 +1662,13 @@ pulls.switch_comparison_type = Switch comparison type pulls.switch_head_and_base = Switch head and base pulls.filter_branch = Filter branch pulls.no_results = No results found. +pulls.show_all_commits = Show all commits +pulls.show_changes_since_your_last_review = Show changes since your last review +pulls.showing_only_single_commit = Showing only changes of commit %[1]s +pulls.showing_specified_commit_range = Showing only changes between %[1]s..%[2]s +pulls.select_commit_hold_shift_for_range = Select commit. Hold shift + click to select a range +pulls.review_only_possible_for_full_diff = Review is only possible when viewing the full diff +pulls.filter_changes_by_commit = Filter by commit pulls.nothing_to_compare = These branches are equal. There is no need to create a pull request. pulls.nothing_to_compare_and_allow_empty_pr = These branches are equal. This PR will be empty. pulls.has_pull_request = `A pull request between these branches already exists: %[2]s#%[3]d` diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 237e53413..a24ef4336 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -694,6 +694,42 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C return compareInfo } +type pullCommitList struct { + Commits []pull_service.CommitInfo `json:"commits"` + LastReviewCommitSha string `json:"last_review_commit_sha"` + Locale map[string]string `json:"locale"` +} + +// GetPullCommits get all commits for given pull request +func GetPullCommits(ctx *context.Context) { + issue := checkPullInfo(ctx) + if ctx.Written() { + return + } + resp := &pullCommitList{} + + commits, lastReviewCommitSha, err := pull_service.GetPullCommits(ctx, issue) + if err != nil { + ctx.JSON(http.StatusInternalServerError, err) + return + } + + // Get the needed locale + resp.Locale = map[string]string{ + "lang": ctx.Locale.Language(), + "filter_changes_by_commit": ctx.Tr("repo.pulls.filter_changes_by_commit"), + "show_all_commits": ctx.Tr("repo.pulls.show_all_commits"), + "stats_num_commits": ctx.TrN(len(commits), "repo.activity.git_stats_commit_1", "repo.activity.git_stats_commit_n", len(commits)), + "show_changes_since_your_last_review": ctx.Tr("repo.pulls.show_changes_since_your_last_review"), + "select_commit_hold_shift_for_range": ctx.Tr("repo.pulls.select_commit_hold_shift_for_range"), + } + + resp.Commits = commits + resp.LastReviewCommitSha = lastReviewCommitSha + + ctx.JSON(http.StatusOK, resp) +} + // ViewPullCommits show commits for a pull request func ViewPullCommits(ctx *context.Context) { ctx.Data["PageIsPullList"] = true @@ -739,7 +775,7 @@ func ViewPullCommits(ctx *context.Context) { } // ViewPullFiles render pull request changed files list page -func ViewPullFiles(ctx *context.Context) { +func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommit string, willShowSpecifiedCommitRange, willShowSpecifiedCommit bool) { ctx.Data["PageIsPullList"] = true ctx.Data["PageIsPullFiles"] = true @@ -762,6 +798,33 @@ func ViewPullFiles(ctx *context.Context) { prInfo = PrepareViewPullInfo(ctx, issue) } + // 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("Given SHA1 not found for this PR", nil) + return + } + } + if ctx.Written() { return } else if prInfo == nil { @@ -775,12 +838,30 @@ func ViewPullFiles(ctx *context.Context) { return } - startCommitID = prInfo.MergeBase - endCommitID = headCommitID + ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit + + if willShowSpecifiedCommit || willShowSpecifiedCommitRange { + if len(specifiedEndCommit) > 0 { + endCommitID = specifiedEndCommit + } else { + endCommitID = headCommitID + } + if len(specifiedStartCommit) > 0 { + startCommitID = specifiedStartCommit + } else { + startCommitID = prInfo.MergeBase + } + ctx.Data["IsShowingAllCommits"] = false + } else { + endCommitID = headCommitID + startCommitID = prInfo.MergeBase + ctx.Data["IsShowingAllCommits"] = true + } 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") @@ -789,8 +870,8 @@ func ViewPullFiles(ctx *context.Context) { if fileOnly && (len(files) == 2 || len(files) == 1) { maxLines, maxFiles = -1, -1 } + diffOptions := &gitdiff.DiffOptions{ - BeforeCommitID: startCommitID, AfterCommitID: endCommitID, SkipTo: ctx.FormString("skip-to"), MaxLines: maxLines, @@ -799,9 +880,18 @@ func ViewPullFiles(ctx *context.Context) { WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), } + if !willShowSpecifiedCommit { + diffOptions.BeforeCommitID = startCommitID + } + var methodWithError string var diff *gitdiff.Diff - if !ctx.IsSigned { + + // if we're not logged in or only a single commit (or commit range) is shown we + // have to load only the diff and not get the viewed information + // as the viewed information is designed to be loaded only on latest PR + // diff and if you're signed in. + if !ctx.IsSigned || willShowSpecifiedCommit || willShowSpecifiedCommitRange { diff, err = gitdiff.GetDiff(gitRepo, diffOptions, files...) methodWithError = "GetDiff" } else { @@ -908,6 +998,22 @@ func ViewPullFiles(ctx *context.Context) { ctx.HTML(http.StatusOK, tplPullFiles) } +func ViewPullFilesForSingleCommit(ctx *context.Context) { + viewPullFiles(ctx, "", ctx.Params("sha"), true, true) +} + +func ViewPullFilesForRange(ctx *context.Context) { + viewPullFiles(ctx, ctx.Params("shaFrom"), ctx.Params("shaTo"), true, false) +} + +func ViewPullFilesStartingFromCommit(ctx *context.Context) { + viewPullFiles(ctx, "", ctx.Params("sha"), true, false) +} + +func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) { + viewPullFiles(ctx, "", "", false, false) +} + // UpdatePullRequest merge PR's baseBranch into headBranch func UpdatePullRequest(ctx *context.Context) { issue := checkPullInfo(ctx) diff --git a/routers/web/user/home_test.go b/routers/web/user/home_test.go index 3a06a38c2..634a91545 100644 --- a/routers/web/user/home_test.go +++ b/routers/web/user/home_test.go @@ -75,7 +75,7 @@ func TestPulls(t *testing.T) { Pulls(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) - assert.Len(t, ctx.Data["Issues"], 4) + assert.Len(t, ctx.Data["Issues"], 5) } func TestMilestones(t *testing.T) { diff --git a/routers/web/web.go b/routers/web/web.go index 0b5196144..ffc26dc29 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1279,14 +1279,20 @@ func registerRoutes(m *web.Route) { m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewIssue) m.Get(".diff", repo.DownloadPullDiff) m.Get(".patch", repo.DownloadPullPatch) - m.Get("/commits", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) + m.Group("/commits", func() { + m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) + m.Get("/list", context.RepoRef(), repo.GetPullCommits) + m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), 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) m.Post("/update", repo.UpdatePullRequest) m.Post("/set_allow_maintainer_edit", web.Bind(forms.UpdateAllowEditsForm{}), repo.SetAllowEdits) m.Post("/cleanup", context.RepoMustNotBeArchived(), context.RepoRef(), repo.CleanUpPullRequest) m.Group("/files", func() { - m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFiles) + m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr) + m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit) + m.Get("/{shaFrom:[a-f0-9]{7,40}}..{shaTo:[a-f0-9]{7,40}}", context.RepoRef(), 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) diff --git a/services/pull/pull.go b/services/pull/pull.go index 3c6caec88..8c0b65fd8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -10,6 +10,7 @@ import ( "os" "regexp" "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" @@ -17,7 +18,9 @@ import ( issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/container" + gitea_context "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/json" @@ -856,3 +859,71 @@ func IsHeadEqualWithBranch(ctx context.Context, pr *issues_model.PullRequest, br } return baseCommit.HasPreviousCommit(headCommit.ID) } + +type CommitInfo struct { + Summary string `json:"summary"` + CommitterOrAuthorName string `json:"committer_or_author_name"` + ID string `json:"id"` + ShortSha string `json:"short_sha"` + Time string `json:"time"` +} + +// GetPullCommits returns all commits on given pull request and the last review commit sha +func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]CommitInfo, string, error) { + pull := issue.PullRequest + + baseGitRepo := ctx.Repo.GitRepo + + if err := pull.LoadBaseRepo(ctx); err != nil { + return nil, "", err + } + baseBranch := pull.BaseBranch + if pull.HasMerged { + baseBranch = pull.MergeBase + } + prInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(), baseBranch, pull.GetGitRefName(), true, false) + if err != nil { + return nil, "", err + } + + commits := make([]CommitInfo, 0, len(prInfo.Commits)) + + for _, commit := range prInfo.Commits { + var committerOrAuthorName string + var commitTime time.Time + if commit.Committer != nil { + committerOrAuthorName = commit.Committer.Name + commitTime = commit.Committer.When + } else { + committerOrAuthorName = commit.Author.Name + commitTime = commit.Author.When + } + + commits = append(commits, CommitInfo{ + Summary: commit.Summary(), + CommitterOrAuthorName: committerOrAuthorName, + ID: commit.ID.String(), + ShortSha: base.ShortSha(commit.ID.String()), + Time: commitTime.Format(time.RFC3339), + }) + } + + var lastReviewCommitID string + if ctx.IsSigned { + // get last review of current user and store information in context (if available) + lastreview, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{ + IssueID: issue.ID, + ReviewerID: ctx.Doer.ID, + Type: issues_model.ReviewTypeUnknown, + }) + + if err != nil && !issues_model.IsErrReviewNotExist(err) { + return nil, "", err + } + if len(lastreview) > 0 { + lastReviewCommitID = lastreview[0].CommitID + } + } + + return commits, lastReviewCommitID, nil +} diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 72e673125..c4dd1f658 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -31,12 +31,32 @@ {{end}} {{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"}} +
+
+ {{end}} {{if and .PageIsPullFiles $.SignedUserID (not .IsArchived)}} {{template "repo/diff/new_review" .}} {{end}} {{if not .DiffNotAvailable}} + {{if and .IsShowingOnlySingleCommit .PageIsPullFiles}} +
+
{{.locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .BeforeCommitID)}} - {{.locale.Tr "repo.pulls.show_all_commits"}}
+
+ {{else if and (not .IsShowingAllCommits) .PageIsPullFiles}} +
+
{{.locale.Tr "repo.pulls.showing_specified_commit_range" (ShortSha .BeforeCommitID) (ShortSha .AfterCommitID)}} - {{.locale.Tr "repo.pulls.show_all_commits"}}
+
+ {{end}} + diff --git a/web_src/js/features/repo-diff-commitselect.js b/web_src/js/features/repo-diff-commitselect.js new file mode 100644 index 000000000..ebac64e85 --- /dev/null +++ b/web_src/js/features/repo-diff-commitselect.js @@ -0,0 +1,10 @@ +import {createApp} from 'vue'; +import DiffCommitSelector from '../components/DiffCommitSelector.vue'; + +export function initDiffCommitSelect() { + const el = document.getElementById('diff-commit-select'); + if (!el) return; + + const commitSelect = createApp(DiffCommitSelector); + commitSelect.mount(el); +} diff --git a/web_src/js/features/repo-diff.js b/web_src/js/features/repo-diff.js index f4bb724fe..b79ca0f5b 100644 --- a/web_src/js/features/repo-diff.js +++ b/web_src/js/features/repo-diff.js @@ -2,6 +2,7 @@ import $ from 'jquery'; import {initCompReactionSelector} from './comp/ReactionSelector.js'; import {initRepoIssueContentHistory} from './repo-issue-content.js'; import {initDiffFileTree} from './repo-diff-filetree.js'; +import {initDiffCommitSelect} from './repo-diff-commitselect.js'; import {validateTextareaNonEmpty} from './comp/ComboMarkdownEditor.js'; import {initViewedCheckboxListenerFor, countAndUpdateViewedFiles, initExpandAndCollapseFilesButton} from './pull-view-file.js'; import {initImageDiff} from './imagediff.js'; @@ -188,6 +189,7 @@ export function initRepoDiffView() { const diffFileList = $('#diff-file-list'); if (diffFileList.length === 0) return; initDiffFileTree(); + initDiffCommitSelect(); initRepoDiffShowMore(); initRepoDiffReviewButton(); initRepoDiffFileViewToggle(); diff --git a/web_src/js/svg.js b/web_src/js/svg.js index 2ef839aa2..46372e7d6 100644 --- a/web_src/js/svg.js +++ b/web_src/js/svg.js @@ -29,6 +29,7 @@ import octiconFileDirectoryFill from '../../public/assets/img/svg/octicon-file-d import octiconFilter from '../../public/assets/img/svg/octicon-filter.svg'; import octiconGear from '../../public/assets/img/svg/octicon-gear.svg'; import octiconGitBranch from '../../public/assets/img/svg/octicon-git-branch.svg'; +import octiconGitCommit from '../../public/assets/img/svg/octicon-git-commit.svg'; import octiconGitMerge from '../../public/assets/img/svg/octicon-git-merge.svg'; import octiconGitPullRequest from '../../public/assets/img/svg/octicon-git-pull-request.svg'; import octiconHeading from '../../public/assets/img/svg/octicon-heading.svg'; @@ -99,6 +100,7 @@ const svgs = { 'octicon-filter': octiconFilter, 'octicon-gear': octiconGear, 'octicon-git-branch': octiconGitBranch, + 'octicon-git-commit': octiconGitCommit, 'octicon-git-merge': octiconGitMerge, 'octicon-git-pull-request': octiconGitPullRequest, 'octicon-heading': octiconHeading,