Skip to content

Commit 6e64f9d

Browse files
lafriksjonasfranz
authored andcommitted
Pull request review/approval and comment on code (#3748)
* Initial ui components for pull request review * Add Review Add IssueComment types Signed-off-by: Jonas Franz <[email protected]> (cherry picked from commit 2b4daab) Signed-off-by: Jonas Franz <[email protected]> * Replace ReviewComment with Content Signed-off-by: Jonas Franz <[email protected]> * Add load functions Add ReviewID to findComments Signed-off-by: Jonas Franz <[email protected]> * Add create review comment implementation Add migration for review Other small changes Signed-off-by: Jonas Franz <[email protected]> * Simplified create and find functions for review Signed-off-by: Jonas Franz <[email protected]> * Moved "Pending" to first position Signed-off-by: Jonas Franz <[email protected]> * Add GetCurrentReview to simplify fetching current review Signed-off-by: Jonas Franz <[email protected]> * Preview for listing comments Signed-off-by: Jonas Franz <[email protected]> * Move new comment form to its own file Signed-off-by: Jonas Franz <[email protected]> * Implement Review form Show Review comments on comment stream Signed-off-by: Jonas Franz <[email protected]> * Add support for single comments Showing buttons in context Signed-off-by: Jonas Franz <[email protected]> * Add pending tag to pending review comments Signed-off-by: Jonas Franz <[email protected]> * Add unit tests for Review Signed-off-by: Jonas Franz <[email protected]> * Fetch all review ids at once Add unit tests Signed-off-by: Jonas Franz <[email protected]> * gofmt Signed-off-by: Jonas Franz <[email protected]> * Improved comment rendering in "Files" view by adding Comments to DiffLine Signed-off-by: Jonas Franz <[email protected]> * Add support for invalidating comments Signed-off-by: Jonas Franz <[email protected]> * Switched back to code.gitea.io/git Signed-off-by: Jonas Franz <[email protected]> * Moved review migration from v64 to v65 Signed-off-by: Jonas Franz <[email protected]> * Rebuild css Signed-off-by: Jonas Franz <[email protected]> * gofmt Signed-off-by: Jonas Franz <[email protected]> * Improve translations Signed-off-by: Jonas Franz <[email protected]> * Fix unit tests by updating fixtures and updating outdated test Signed-off-by: Jonas Franz <[email protected]> * Comments will be shown at the right place now Signed-off-by: Jonas Franz <[email protected]> * Add support for deleting CodeComments Signed-off-by: Jonas Franz <[email protected]> * Fix problems caused by files in subdirectories Signed-off-by: Jonas Franz <[email protected]> * Add support for showing code comments of reviews in conversation Signed-off-by: Jonas Franz <[email protected]> * Add support for "Show/Hide outdated" Signed-off-by: Jonas Franz <[email protected]> * Update code.gitea.io/git Signed-off-by: Jonas Franz <[email protected]> * Add support for new webhooks Signed-off-by: Jonas Franz <[email protected]> * Update comparison Signed-off-by: Jonas Franz <[email protected]> * Resolve conflicts Signed-off-by: Jonas Franz <[email protected]> * Minor UI improvements * update code.gitea.io/git * Fix ui bug reported by @lunny causing wrong position of add button Add functionality to "Cancel" button Add scale effects to add button Hide "Cancel" button for existing comments Signed-off-by: Jonas Franz <[email protected]> * Prepare solving conflicts Signed-off-by: Jonas Franz <[email protected]> * Show add button only if no comments already exist for the line Signed-off-by: Jonas Franz <[email protected]> * Add missing vendor files Signed-off-by: Jonas Franz <[email protected]> * Check if reviewer is nil Signed-off-by: Jonas Franz <[email protected]> * Show forms only to users who are logged in Signed-off-by: Jonas Franz <[email protected]> * Revert "Show forms only to users who are logged in" This reverts commit c083682 Signed-off-by: Jonas Franz <[email protected]> * Save patch in comment Render patch for code comments Signed-off-by: Jonas Franz <[email protected]> * Add link to comment in code Signed-off-by: Jonas Franz <[email protected]> * Add reply form to comment list Show forms only to signed in users Signed-off-by: Jonas Franz <[email protected]> * Add 'Reply' as translatable Add CODE_COMMENT_LINES setting Signed-off-by: Jonas Franz <[email protected]> * gofmt Signed-off-by: Jonas Franz <[email protected]> * Fix problems introduced by checking for singed in user Signed-off-by: Jonas Franz <[email protected]> * Add v70 Signed-off-by: Jonas Franz <[email protected]> * Update generated stylesheet Signed-off-by: Jonas Franz <[email protected]> * Fix preview Beginn with new review comment patch system Signed-off-by: Jonas Franz <[email protected]> * Add new algo to generate diff for line range Remove old algo used for cutting big diffs (it was very buggy) * Add documentation and example for CutDiffAroundLine * Fix example of CutDiffAroundLine * Fix some comment UI rendering bugs * Add code comment edit mode * Send notifications / actions to users until review gets published Fix diff generation bug Fix wrong hashtag * Fix vet errors * Send notifications also for single comments * Fix some notification bugs, fix link * Fix: add comment icon is only shown on code lines * Add lint comment * Add unit tests for git diff * Add more error messages * Regenerated css Signed-off-by: Jonas Franz <[email protected]> * fmt Signed-off-by: Jonas Franz <[email protected]> * Regenerated CSS with latest less version Signed-off-by: Jonas Franz <[email protected]> * Fix test by updating comment type to new ID Signed-off-by: Jonas Franz <[email protected]> * Introducing CodeComments as type for map[string]map[int64][]*Comment Other minor code improvements Signed-off-by: Jonas Franz <[email protected]> * Fix data-tab issues Signed-off-by: Jonas Franz <[email protected]> * Remove unnecessary change Signed-off-by: Jonas Franz <[email protected]> * refactored checkForInvalidation Signed-off-by: Jonas Franz <[email protected]> * Append comments instead of setting Signed-off-by: Jonas Franz <[email protected]> * Use HeadRepo instead of BaseRepo Signed-off-by: Jonas Franz <[email protected]> * Update migration Signed-off-by: Jonas Franz <[email protected]> * Regenerated CSS Signed-off-by: Jonas Franz <[email protected]> * Add copyright Signed-off-by: Jonas Franz <[email protected]> * Update index.css Signed-off-by: Jonas Franz <[email protected]>
1 parent 9c354a5 commit 6e64f9d

40 files changed

+2010
-89
lines changed

custom/conf/app.ini.sample

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ ISSUE_PAGING_NUM = 10
6969
FEED_MAX_COMMIT_NUM = 5
7070
; Number of maximum commits displayed in commit graph.
7171
GRAPH_MAX_COMMIT_NUM = 100
72+
; Number of line of codes shown for a code comment
73+
CODE_COMMENT_LINES = 4
7274
; Value of `theme-color` meta tag, used by Android >= 5.0
7375
; An invalid color like "none" or "disable" will have the default style
7476
; More info: https://developers.google.com/web/updates/2014/11/Support-for-theme-color-in-Chrome-39-for-Android

docs/content/doc/features/comparison.en-us.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,8 @@ _Symbols used in table:_
9292
| Pull/Merge requests ||||||||
9393
| Squash merging ||||||||
9494
| Rebase merging ||||||||
95-
| Pull/Merge request inline comments | |||||||
96-
| Pull/Merge request approval | |||||||
95+
| Pull/Merge request inline comments | |||||||
96+
| Pull/Merge request approval | |||||||
9797
| Merge conflict resolution ||||||||
9898
| Restrict push and merge access to certain users ||||||||
9999
| Revert specific commits or a merge request ||||||||

models/error.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1344,3 +1344,25 @@ func IsErrUnknownDependencyType(err error) bool {
13441344
func (err ErrUnknownDependencyType) Error() string {
13451345
return fmt.Sprintf("unknown dependency type [type: %d]", err.Type)
13461346
}
1347+
1348+
// __________ .__
1349+
// \______ \ _______ _|__| ______ _ __
1350+
// | _// __ \ \/ / |/ __ \ \/ \/ /
1351+
// | | \ ___/\ /| \ ___/\ /
1352+
// |____|_ /\___ >\_/ |__|\___ >\/\_/
1353+
// \/ \/ \/
1354+
1355+
// ErrReviewNotExist represents a "ReviewNotExist" kind of error.
1356+
type ErrReviewNotExist struct {
1357+
ID int64
1358+
}
1359+
1360+
// IsErrReviewNotExist checks if an error is a ErrReviewNotExist.
1361+
func IsErrReviewNotExist(err error) bool {
1362+
_, ok := err.(ErrReviewNotExist)
1363+
return ok
1364+
}
1365+
1366+
func (err ErrReviewNotExist) Error() string {
1367+
return fmt.Sprintf("review does not exist [id: %d]", err.ID)
1368+
}

models/fixtures/comment.yml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,24 @@
2020
issue_id: 1 # in repo_id 1
2121
content: "meh..."
2222
created_unix: 946684812
23+
-
24+
id: 4
25+
type: 21 # code comment
26+
poster_id: 1
27+
issue_id: 2
28+
content: "meh..."
29+
review_id: 4
30+
line: 4
31+
tree_path: "README.md"
32+
created_unix: 946684812
33+
invalidated: false
34+
-
35+
id: 5
36+
type: 21 # code comment
37+
poster_id: 1
38+
issue_id: 2
39+
content: "meh..."
40+
line: -4
41+
tree_path: "README.md"
42+
created_unix: 946684812
43+
invalidated: false

models/fixtures/review.yml

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
-
2+
id: 1
3+
type: 1
4+
reviewer_id: 1
5+
issue_id: 2
6+
content: "Demo Review"
7+
updated_unix: 946684810
8+
created_unix: 946684810
9+
-
10+
id: 2
11+
type: 1
12+
reviewer_id: 534543
13+
issue_id: 534543
14+
content: "Invalid Review #1"
15+
updated_unix: 946684810
16+
created_unix: 946684810
17+
-
18+
id: 3
19+
type: 1
20+
reviewer_id: 1
21+
issue_id: 343545
22+
content: "Invalid Review #2"
23+
updated_unix: 946684810
24+
created_unix: 946684810
25+
-
26+
id: 4
27+
type: 0 # Pending review
28+
reviewer_id: 1
29+
issue_id: 2
30+
content: "Pending Review"
31+
updated_unix: 946684810
32+
created_unix: 946684810

models/git_diff.go

Lines changed: 195 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import (
1414
"io/ioutil"
1515
"os"
1616
"os/exec"
17+
"regexp"
18+
"sort"
1719
"strconv"
1820
"strings"
1921

@@ -57,13 +59,27 @@ type DiffLine struct {
5759
RightIdx int
5860
Type DiffLineType
5961
Content string
62+
Comments []*Comment
6063
}
6164

6265
// GetType returns the type of a DiffLine.
6366
func (d *DiffLine) GetType() int {
6467
return int(d.Type)
6568
}
6669

70+
// CanComment returns whether or not a line can get commented
71+
func (d *DiffLine) CanComment() bool {
72+
return len(d.Comments) == 0 && d.Type != DiffLineSection
73+
}
74+
75+
// GetCommentSide returns the comment side of the first comment, if not set returns empty string
76+
func (d *DiffLine) GetCommentSide() string {
77+
if len(d.Comments) == 0 {
78+
return ""
79+
}
80+
return d.Comments[0].DiffSide()
81+
}
82+
6783
// DiffSection represents a section of a DiffFile.
6884
type DiffSection struct {
6985
Name string
@@ -225,11 +241,167 @@ type Diff struct {
225241
IsIncomplete bool
226242
}
227243

244+
// LoadComments loads comments into each line
245+
func (diff *Diff) LoadComments(issue *Issue, currentUser *User) error {
246+
allComments, err := FetchCodeComments(issue, currentUser)
247+
if err != nil {
248+
return err
249+
}
250+
for _, file := range diff.Files {
251+
if lineCommits, ok := allComments[file.Name]; ok {
252+
for _, section := range file.Sections {
253+
for _, line := range section.Lines {
254+
if comments, ok := lineCommits[int64(line.LeftIdx*-1)]; ok {
255+
line.Comments = append(line.Comments, comments...)
256+
}
257+
if comments, ok := lineCommits[int64(line.RightIdx)]; ok {
258+
line.Comments = append(line.Comments, comments...)
259+
}
260+
sort.SliceStable(line.Comments, func(i, j int) bool {
261+
return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix
262+
})
263+
}
264+
}
265+
}
266+
}
267+
return nil
268+
}
269+
228270
// NumFiles returns number of files changes in a diff.
229271
func (diff *Diff) NumFiles() int {
230272
return len(diff.Files)
231273
}
232274

275+
// Example: @@ -1,8 +1,9 @@ => [..., 1, 8, 1, 9]
276+
var hunkRegex = regexp.MustCompile(`^@@ -([0-9]+),([0-9]+) \+([0-9]+),([0-9]+) @@`)
277+
278+
func isHeader(lof string) bool {
279+
return strings.HasPrefix(lof, cmdDiffHead) || strings.HasPrefix(lof, "---") || strings.HasPrefix(lof, "+++")
280+
}
281+
282+
// CutDiffAroundLine cuts a diff of a file in way that only the given line + numberOfLine above it will be shown
283+
// it also recalculates hunks and adds the appropriate headers to the new diff.
284+
// Warning: Only one-file diffs are allowed.
285+
func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLine int) string {
286+
if line == 0 || numbersOfLine == 0 {
287+
// no line or num of lines => no diff
288+
return ""
289+
}
290+
scanner := bufio.NewScanner(originalDiff)
291+
hunk := make([]string, 0)
292+
// begin is the start of the hunk containing searched line
293+
// end is the end of the hunk ...
294+
// currentLine is the line number on the side of the searched line (differentiated by old)
295+
// otherLine is the line number on the opposite side of the searched line (differentiated by old)
296+
var begin, end, currentLine, otherLine int64
297+
var headerLines int
298+
for scanner.Scan() {
299+
lof := scanner.Text()
300+
// Add header to enable parsing
301+
if isHeader(lof) {
302+
hunk = append(hunk, lof)
303+
headerLines++
304+
}
305+
if currentLine > line {
306+
break
307+
}
308+
// Detect "hunk" with contains commented lof
309+
if strings.HasPrefix(lof, "@@") {
310+
// Already got our hunk. End of hunk detected!
311+
if len(hunk) > headerLines {
312+
break
313+
}
314+
groups := hunkRegex.FindStringSubmatch(lof)
315+
if old {
316+
begin = com.StrTo(groups[1]).MustInt64()
317+
end = com.StrTo(groups[2]).MustInt64()
318+
// init otherLine with begin of opposite side
319+
otherLine = com.StrTo(groups[3]).MustInt64()
320+
} else {
321+
begin = com.StrTo(groups[3]).MustInt64()
322+
end = com.StrTo(groups[4]).MustInt64()
323+
// init otherLine with begin of opposite side
324+
otherLine = com.StrTo(groups[1]).MustInt64()
325+
}
326+
end += begin // end is for real only the number of lines in hunk
327+
// lof is between begin and end
328+
if begin <= line && end >= line {
329+
hunk = append(hunk, lof)
330+
currentLine = begin
331+
continue
332+
}
333+
} else if len(hunk) > headerLines {
334+
hunk = append(hunk, lof)
335+
// Count lines in context
336+
switch lof[0] {
337+
case '+':
338+
if !old {
339+
currentLine++
340+
} else {
341+
otherLine++
342+
}
343+
case '-':
344+
if old {
345+
currentLine++
346+
} else {
347+
otherLine++
348+
}
349+
default:
350+
currentLine++
351+
otherLine++
352+
}
353+
}
354+
}
355+
356+
// No hunk found
357+
if currentLine == 0 {
358+
return ""
359+
}
360+
// headerLines + hunkLine (1) = totalNonCodeLines
361+
if len(hunk)-headerLines-1 <= numbersOfLine {
362+
// No need to cut the hunk => return existing hunk
363+
return strings.Join(hunk, "\n")
364+
}
365+
var oldBegin, oldNumOfLines, newBegin, newNumOfLines int64
366+
if old {
367+
oldBegin = currentLine
368+
newBegin = otherLine
369+
} else {
370+
oldBegin = otherLine
371+
newBegin = currentLine
372+
}
373+
// headers + hunk header
374+
newHunk := make([]string, headerLines)
375+
// transfer existing headers
376+
for idx, lof := range hunk[:headerLines] {
377+
newHunk[idx] = lof
378+
}
379+
// transfer last n lines
380+
for _, lof := range hunk[len(hunk)-numbersOfLine-1:] {
381+
newHunk = append(newHunk, lof)
382+
}
383+
// calculate newBegin, ... by counting lines
384+
for i := len(hunk) - 1; i >= len(hunk)-numbersOfLine; i-- {
385+
switch hunk[i][0] {
386+
case '+':
387+
newBegin--
388+
newNumOfLines++
389+
case '-':
390+
oldBegin--
391+
oldNumOfLines++
392+
default:
393+
oldBegin--
394+
newBegin--
395+
newNumOfLines++
396+
oldNumOfLines++
397+
}
398+
}
399+
// construct the new hunk header
400+
newHunk[headerLines] = fmt.Sprintf("@@ -%d,%d +%d,%d @@",
401+
oldBegin, oldNumOfLines, newBegin, newNumOfLines)
402+
return strings.Join(newHunk, "\n")
403+
}
404+
233405
const cmdDiffHead = "diff --git "
234406

235407
// ParsePatch builds a Diff object from a io.Reader and some
@@ -307,7 +479,6 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
307479
if curFileLinesCount >= maxLines {
308480
curFile.IsIncomplete = true
309481
}
310-
311482
switch {
312483
case line[0] == ' ':
313484
diffLine := &DiffLine{Type: DiffLinePlain, Content: line, LeftIdx: leftLine, RightIdx: rightLine}
@@ -524,32 +695,46 @@ const (
524695
// GetRawDiff dumps diff results of repository in given commit ID to io.Writer.
525696
// TODO: move this function to gogits/git-module
526697
func GetRawDiff(repoPath, commitID string, diffType RawDiffType, writer io.Writer) error {
698+
return GetRawDiffForFile(repoPath, "", commitID, diffType, "", writer)
699+
}
700+
701+
// GetRawDiffForFile dumps diff results of file in given commit ID to io.Writer.
702+
// TODO: move this function to gogits/git-module
703+
func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiffType, file string, writer io.Writer) error {
527704
repo, err := git.OpenRepository(repoPath)
528705
if err != nil {
529706
return fmt.Errorf("OpenRepository: %v", err)
530707
}
531708

532-
commit, err := repo.GetCommit(commitID)
709+
commit, err := repo.GetCommit(endCommit)
533710
if err != nil {
534711
return fmt.Errorf("GetCommit: %v", err)
535712
}
536-
713+
fileArgs := make([]string, 0)
714+
if len(file) > 0 {
715+
fileArgs = append(fileArgs, "--", file)
716+
}
537717
var cmd *exec.Cmd
538718
switch diffType {
539719
case RawDiffNormal:
540-
if commit.ParentCount() == 0 {
541-
cmd = exec.Command("git", "show", commitID)
720+
if len(startCommit) != 0 {
721+
cmd = exec.Command("git", append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...)
722+
} else if commit.ParentCount() == 0 {
723+
cmd = exec.Command("git", append([]string{"show", endCommit}, fileArgs...)...)
542724
} else {
543725
c, _ := commit.Parent(0)
544-
cmd = exec.Command("git", "diff", "-M", c.ID.String(), commitID)
726+
cmd = exec.Command("git", append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...)
545727
}
546728
case RawDiffPatch:
547-
if commit.ParentCount() == 0 {
548-
cmd = exec.Command("git", "format-patch", "--no-signature", "--stdout", "--root", commitID)
729+
if len(startCommit) != 0 {
730+
query := fmt.Sprintf("%s...%s", endCommit, startCommit)
731+
cmd = exec.Command("git", append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...)
732+
} else if commit.ParentCount() == 0 {
733+
cmd = exec.Command("git", append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...)
549734
} else {
550735
c, _ := commit.Parent(0)
551-
query := fmt.Sprintf("%s...%s", commitID, c.ID.String())
552-
cmd = exec.Command("git", "format-patch", "--no-signature", "--stdout", query)
736+
query := fmt.Sprintf("%s...%s", endCommit, c.ID.String())
737+
cmd = exec.Command("git", append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...)
553738
}
554739
default:
555740
return fmt.Errorf("invalid diffType: %s", diffType)
@@ -560,7 +745,6 @@ func GetRawDiff(repoPath, commitID string, diffType RawDiffType, writer io.Write
560745
cmd.Dir = repoPath
561746
cmd.Stdout = writer
562747
cmd.Stderr = stderr
563-
564748
if err = cmd.Run(); err != nil {
565749
return fmt.Errorf("Run: %v - %s", err, stderr)
566750
}

0 commit comments

Comments
 (0)