Skip to content

Commit 5553206

Browse files
sebastian-sauersilverwindKN4CK3Rwxiaoguangdelvh
authored
Add commits dropdown in PR files view and allow commit by commit review (#25528)
This PR adds a new dropdown to select a commit or a commit range (shift-click like github) of a Pull Request. After selection of a commit only the changes of this commit will be shown. When selecting a range of commits the diff of this range is shown. This allows to review a PR commit by commit or by viewing only commit ranges. The "Show changes since your last review" mechanism github uses is implemented, too. When reviewing a single commit or a commit range the "Viewed" functionality is disabled. ## Screenshots ### The commit dropdown ![image](https://github.com/go-gitea/gitea/assets/51889757/0db3ae62-1272-436c-be64-4730c5d611e3) ### Selecting a commit range ![image](https://github.com/go-gitea/gitea/assets/51889757/ad81eedb-8437-42b0-8073-2d940c25fe8f) ### Show changes of a single commit only ![image](https://github.com/go-gitea/gitea/assets/51889757/6b1a113b-73ef-4ecc-adf6-bc2340bb8f97) ### Show changes of a commit range ![image](https://github.com/go-gitea/gitea/assets/51889757/6401b358-cd66-4c09-8baa-6cf6177f23a7) Fixes #20989 Fixes #19263 --------- Co-authored-by: silverwind <[email protected]> Co-authored-by: KN4CK3R <[email protected]> Co-authored-by: wxiaoguang <[email protected]> Co-authored-by: delvh <[email protected]>
1 parent 4971a10 commit 5553206

File tree

71 files changed

+748
-35
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

71 files changed

+748
-35
lines changed

models/fixtures/issue.yml

+17
Original file line numberDiff line numberDiff line change
@@ -304,3 +304,20 @@
304304
created_unix: 946684830
305305
updated_unix: 978307200
306306
is_locked: false
307+
308+
-
309+
id: 19
310+
repo_id: 58
311+
index: 1
312+
poster_id: 2
313+
original_author_id: 0
314+
name: issue for pr
315+
content: content
316+
milestone_id: 0
317+
priority: 0
318+
is_closed: false
319+
is_pull: true
320+
num_comments: 0
321+
created_unix: 946684830
322+
updated_unix: 978307200
323+
is_locked: false

models/fixtures/pull_request.yml

+13
Original file line numberDiff line numberDiff line change
@@ -76,3 +76,16 @@
7676
base_branch: master
7777
merge_base: 2a47ca4b614a9f5a
7878
has_merged: false
79+
80+
-
81+
id: 7
82+
type: 0 # gitea pull request
83+
status: 2 # mergable
84+
issue_id: 19
85+
index: 1
86+
head_repo_id: 58
87+
base_repo_id: 58
88+
head_branch: branch1
89+
base_branch: main
90+
merge_base: cbff181af4c9c7fee3cf6c106699e07d9a3f54e6
91+
has_merged: false

models/fixtures/repo_unit.yml

+30
Original file line numberDiff line numberDiff line change
@@ -607,3 +607,33 @@
607607
repo_id: 52
608608
type: 1
609609
created_unix: 946684810
610+
611+
-
612+
id: 91
613+
repo_id: 58
614+
type: 1
615+
created_unix: 946684810
616+
617+
-
618+
id: 92
619+
repo_id: 58
620+
type: 2
621+
created_unix: 946684810
622+
623+
-
624+
id: 93
625+
repo_id: 58
626+
type: 3
627+
created_unix: 946684810
628+
629+
-
630+
id: 94
631+
repo_id: 58
632+
type: 4
633+
created_unix: 946684810
634+
635+
-
636+
id: 95
637+
repo_id: 58
638+
type: 5
639+
created_unix: 946684810

models/fixtures/repository.yml

+31
Original file line numberDiff line numberDiff line change
@@ -1662,3 +1662,34 @@
16621662
is_private: false
16631663
status: 0
16641664
num_issues: 0
1665+
1666+
-
1667+
id: 58 # org public repo
1668+
owner_id: 2
1669+
owner_name: user2
1670+
lower_name: commitsonpr
1671+
name: commitsonpr
1672+
default_branch: main
1673+
num_watches: 0
1674+
num_stars: 0
1675+
num_forks: 0
1676+
num_issues: 0
1677+
num_closed_issues: 0
1678+
num_pulls: 1
1679+
num_closed_pulls: 0
1680+
num_milestones: 0
1681+
num_closed_milestones: 0
1682+
num_projects: 0
1683+
num_closed_projects: 0
1684+
is_private: false
1685+
is_empty: false
1686+
is_archived: false
1687+
is_mirror: false
1688+
status: 0
1689+
is_fork: false
1690+
fork_id: 0
1691+
is_template: false
1692+
template_id: 0
1693+
size: 0
1694+
is_fsck_enabled: true
1695+
close_issues_via_commit_in_any_branch: false

models/fixtures/user.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@
6666
num_followers: 2
6767
num_following: 1
6868
num_stars: 2
69-
num_repos: 13
69+
num_repos: 14
7070
num_teams: 0
7171
num_members: 0
7272
visibility: 0

models/issues/issue_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ func TestCountIssues(t *testing.T) {
538538
assert.NoError(t, unittest.PrepareTestDatabase())
539539
count, err := issues_model.CountIssues(db.DefaultContext, &issues_model.IssuesOptions{})
540540
assert.NoError(t, err)
541-
assert.EqualValues(t, 18, count)
541+
assert.EqualValues(t, 19, count)
542542
}
543543

544544
func TestIssueLoadAttributes(t *testing.T) {

models/issues/review_list.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func FindLatestReviews(ctx context.Context, opts FindReviewOptions) (ReviewList,
114114
}
115115

116116
sess.In("id", builder.
117-
Select("max ( id ) ").
117+
Select("max(id)").
118118
From("review").
119119
Where(cond).
120120
GroupBy("reviewer_id"))

models/repo/repo_list_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -235,12 +235,12 @@ func TestSearchRepository(t *testing.T) {
235235
{
236236
name: "AllPublic/PublicRepositoriesOfUserIncludingCollaborative",
237237
opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: util.OptionalBoolFalse},
238-
count: 30,
238+
count: 31,
239239
},
240240
{
241241
name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
242242
opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: util.OptionalBoolFalse},
243-
count: 35,
243+
count: 36,
244244
},
245245
{
246246
name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
@@ -255,7 +255,7 @@ func TestSearchRepository(t *testing.T) {
255255
{
256256
name: "AllPublic/PublicRepositoriesOfOrganization",
257257
opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: util.OptionalBoolFalse, Template: util.OptionalBoolFalse},
258-
count: 30,
258+
count: 31,
259259
},
260260
{
261261
name: "AllTemplates",

options/locale/locale_en-US.ini

+7
Original file line numberDiff line numberDiff line change
@@ -1662,6 +1662,13 @@ pulls.switch_comparison_type = Switch comparison type
16621662
pulls.switch_head_and_base = Switch head and base
16631663
pulls.filter_branch = Filter branch
16641664
pulls.no_results = No results found.
1665+
pulls.show_all_commits = Show all commits
1666+
pulls.show_changes_since_your_last_review = Show changes since your last review
1667+
pulls.showing_only_single_commit = Showing only changes of commit %[1]s
1668+
pulls.showing_specified_commit_range = Showing only changes between %[1]s..%[2]s
1669+
pulls.select_commit_hold_shift_for_range = Select commit. Hold shift + click to select a range
1670+
pulls.review_only_possible_for_full_diff = Review is only possible when viewing the full diff
1671+
pulls.filter_changes_by_commit = Filter by commit
16651672
pulls.nothing_to_compare = These branches are equal. There is no need to create a pull request.
16661673
pulls.nothing_to_compare_and_allow_empty_pr = These branches are equal. This PR will be empty.
16671674
pulls.has_pull_request = `A pull request between these branches already exists: <a href="%[1]s">%[2]s#%[3]d</a>`

routers/web/repo/pull.go

+111-5
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,42 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
694694
return compareInfo
695695
}
696696

697+
type pullCommitList struct {
698+
Commits []pull_service.CommitInfo `json:"commits"`
699+
LastReviewCommitSha string `json:"last_review_commit_sha"`
700+
Locale map[string]string `json:"locale"`
701+
}
702+
703+
// GetPullCommits get all commits for given pull request
704+
func GetPullCommits(ctx *context.Context) {
705+
issue := checkPullInfo(ctx)
706+
if ctx.Written() {
707+
return
708+
}
709+
resp := &pullCommitList{}
710+
711+
commits, lastReviewCommitSha, err := pull_service.GetPullCommits(ctx, issue)
712+
if err != nil {
713+
ctx.JSON(http.StatusInternalServerError, err)
714+
return
715+
}
716+
717+
// Get the needed locale
718+
resp.Locale = map[string]string{
719+
"lang": ctx.Locale.Language(),
720+
"filter_changes_by_commit": ctx.Tr("repo.pulls.filter_changes_by_commit"),
721+
"show_all_commits": ctx.Tr("repo.pulls.show_all_commits"),
722+
"stats_num_commits": ctx.TrN(len(commits), "repo.activity.git_stats_commit_1", "repo.activity.git_stats_commit_n", len(commits)),
723+
"show_changes_since_your_last_review": ctx.Tr("repo.pulls.show_changes_since_your_last_review"),
724+
"select_commit_hold_shift_for_range": ctx.Tr("repo.pulls.select_commit_hold_shift_for_range"),
725+
}
726+
727+
resp.Commits = commits
728+
resp.LastReviewCommitSha = lastReviewCommitSha
729+
730+
ctx.JSON(http.StatusOK, resp)
731+
}
732+
697733
// ViewPullCommits show commits for a pull request
698734
func ViewPullCommits(ctx *context.Context) {
699735
ctx.Data["PageIsPullList"] = true
@@ -739,7 +775,7 @@ func ViewPullCommits(ctx *context.Context) {
739775
}
740776

741777
// ViewPullFiles render pull request changed files list page
742-
func ViewPullFiles(ctx *context.Context) {
778+
func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommit string, willShowSpecifiedCommitRange, willShowSpecifiedCommit bool) {
743779
ctx.Data["PageIsPullList"] = true
744780
ctx.Data["PageIsPullFiles"] = true
745781

@@ -762,6 +798,33 @@ func ViewPullFiles(ctx *context.Context) {
762798
prInfo = PrepareViewPullInfo(ctx, issue)
763799
}
764800

801+
// Validate the given commit sha to show (if any passed)
802+
if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
803+
804+
foundStartCommit := len(specifiedStartCommit) == 0
805+
foundEndCommit := len(specifiedEndCommit) == 0
806+
807+
if !(foundStartCommit && foundEndCommit) {
808+
for _, commit := range prInfo.Commits {
809+
if commit.ID.String() == specifiedStartCommit {
810+
foundStartCommit = true
811+
}
812+
if commit.ID.String() == specifiedEndCommit {
813+
foundEndCommit = true
814+
}
815+
816+
if foundStartCommit && foundEndCommit {
817+
break
818+
}
819+
}
820+
}
821+
822+
if !(foundStartCommit && foundEndCommit) {
823+
ctx.NotFound("Given SHA1 not found for this PR", nil)
824+
return
825+
}
826+
}
827+
765828
if ctx.Written() {
766829
return
767830
} else if prInfo == nil {
@@ -775,12 +838,30 @@ func ViewPullFiles(ctx *context.Context) {
775838
return
776839
}
777840

778-
startCommitID = prInfo.MergeBase
779-
endCommitID = headCommitID
841+
ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit
842+
843+
if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
844+
if len(specifiedEndCommit) > 0 {
845+
endCommitID = specifiedEndCommit
846+
} else {
847+
endCommitID = headCommitID
848+
}
849+
if len(specifiedStartCommit) > 0 {
850+
startCommitID = specifiedStartCommit
851+
} else {
852+
startCommitID = prInfo.MergeBase
853+
}
854+
ctx.Data["IsShowingAllCommits"] = false
855+
} else {
856+
endCommitID = headCommitID
857+
startCommitID = prInfo.MergeBase
858+
ctx.Data["IsShowingAllCommits"] = true
859+
}
780860

781861
ctx.Data["Username"] = ctx.Repo.Owner.Name
782862
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
783863
ctx.Data["AfterCommitID"] = endCommitID
864+
ctx.Data["BeforeCommitID"] = startCommitID
784865

785866
fileOnly := ctx.FormBool("file-only")
786867

@@ -789,8 +870,8 @@ func ViewPullFiles(ctx *context.Context) {
789870
if fileOnly && (len(files) == 2 || len(files) == 1) {
790871
maxLines, maxFiles = -1, -1
791872
}
873+
792874
diffOptions := &gitdiff.DiffOptions{
793-
BeforeCommitID: startCommitID,
794875
AfterCommitID: endCommitID,
795876
SkipTo: ctx.FormString("skip-to"),
796877
MaxLines: maxLines,
@@ -799,9 +880,18 @@ func ViewPullFiles(ctx *context.Context) {
799880
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
800881
}
801882

883+
if !willShowSpecifiedCommit {
884+
diffOptions.BeforeCommitID = startCommitID
885+
}
886+
802887
var methodWithError string
803888
var diff *gitdiff.Diff
804-
if !ctx.IsSigned {
889+
890+
// if we're not logged in or only a single commit (or commit range) is shown we
891+
// have to load only the diff and not get the viewed information
892+
// as the viewed information is designed to be loaded only on latest PR
893+
// diff and if you're signed in.
894+
if !ctx.IsSigned || willShowSpecifiedCommit || willShowSpecifiedCommitRange {
805895
diff, err = gitdiff.GetDiff(gitRepo, diffOptions, files...)
806896
methodWithError = "GetDiff"
807897
} else {
@@ -908,6 +998,22 @@ func ViewPullFiles(ctx *context.Context) {
908998
ctx.HTML(http.StatusOK, tplPullFiles)
909999
}
9101000

1001+
func ViewPullFilesForSingleCommit(ctx *context.Context) {
1002+
viewPullFiles(ctx, "", ctx.Params("sha"), true, true)
1003+
}
1004+
1005+
func ViewPullFilesForRange(ctx *context.Context) {
1006+
viewPullFiles(ctx, ctx.Params("shaFrom"), ctx.Params("shaTo"), true, false)
1007+
}
1008+
1009+
func ViewPullFilesStartingFromCommit(ctx *context.Context) {
1010+
viewPullFiles(ctx, "", ctx.Params("sha"), true, false)
1011+
}
1012+
1013+
func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) {
1014+
viewPullFiles(ctx, "", "", false, false)
1015+
}
1016+
9111017
// UpdatePullRequest merge PR's baseBranch into headBranch
9121018
func UpdatePullRequest(ctx *context.Context) {
9131019
issue := checkPullInfo(ctx)

routers/web/user/home_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func TestPulls(t *testing.T) {
7575
Pulls(ctx)
7676
assert.EqualValues(t, http.StatusOK, ctx.Resp.Status())
7777

78-
assert.Len(t, ctx.Data["Issues"], 4)
78+
assert.Len(t, ctx.Data["Issues"], 5)
7979
}
8080

8181
func TestMilestones(t *testing.T) {

routers/web/web.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -1279,14 +1279,20 @@ func registerRoutes(m *web.Route) {
12791279
m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewIssue)
12801280
m.Get(".diff", repo.DownloadPullDiff)
12811281
m.Get(".patch", repo.DownloadPullPatch)
1282-
m.Get("/commits", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits)
1282+
m.Group("/commits", func() {
1283+
m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits)
1284+
m.Get("/list", context.RepoRef(), repo.GetPullCommits)
1285+
m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
1286+
})
12831287
m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), repo.MergePullRequest)
12841288
m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest)
12851289
m.Post("/update", repo.UpdatePullRequest)
12861290
m.Post("/set_allow_maintainer_edit", web.Bind(forms.UpdateAllowEditsForm{}), repo.SetAllowEdits)
12871291
m.Post("/cleanup", context.RepoMustNotBeArchived(), context.RepoRef(), repo.CleanUpPullRequest)
12881292
m.Group("/files", func() {
1289-
m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFiles)
1293+
m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr)
1294+
m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit)
1295+
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)
12901296
m.Group("/reviews", func() {
12911297
m.Get("/new_comment", repo.RenderNewCodeCommentForm)
12921298
m.Post("/comments", web.Bind(forms.CodeCommentForm{}), repo.SetShowOutdatedComments, repo.CreateCodeComment)

0 commit comments

Comments
 (0)