Skip to content

Commit 68df14b

Browse files
nemoinhoFelix Nehrke
nemoinho
authored and
Felix Nehrke
committed
Add whitespace handling to PR-comparsion
In a PR we have to keep an eye on a lot of different things. But sometimes the bare code is the key-thing we want to care about and just don't want to care about fixed indention on some places. Especially if we follow the pathfinder rule we face a lot of these situations because these changes don't break the code in many languages but improve the readability a lot. So this change introduce a fine graned button to adjust the way how the reviewer want to see whitespace-changes within the code. The possibilities reflect the possibilities from git itself except of the `--ignore-blank-lines` flag because that one is also handled by `-b` and is really rare. Signed-off-by: Felix Nehrke <[email protected]>
1 parent 52c2cb1 commit 68df14b

File tree

8 files changed

+112
-15
lines changed

8 files changed

+112
-15
lines changed

‎models/git_diff.go

+21-10
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,13 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
633633
// passing the empty string as beforeCommitID returns a diff from the
634634
// parent commit.
635635
func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int) (*Diff, error) {
636+
return GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID, maxLines, maxLineCharacters, maxFiles, "")
637+
}
638+
639+
// GetDiffRangeWithWhitespaceBehavior builds a Diff between two commits of a repository.
640+
// Passing the empty string as beforeCommitID returns a diff from the parent commit.
641+
// The whitespaceBehavior is either an empty string or a git flag
642+
func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string) (*Diff, error) {
636643
gitRepo, err := git.OpenRepository(repoPath)
637644
if err != nil {
638645
return nil, err
@@ -644,17 +651,21 @@ func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxL
644651
}
645652

646653
var cmd *exec.Cmd
647-
// if "after" commit given
648-
if len(beforeCommitID) == 0 {
649-
// First commit of repository.
650-
if commit.ParentCount() == 0 {
651-
cmd = exec.Command("git", "show", afterCommitID)
652-
} else {
653-
c, _ := commit.Parent(0)
654-
cmd = exec.Command("git", "diff", "-M", c.ID.String(), afterCommitID)
655-
}
654+
if len(beforeCommitID) == 0 && commit.ParentCount() == 0 {
655+
cmd = exec.Command("git", "show", afterCommitID)
656656
} else {
657-
cmd = exec.Command("git", "diff", "-M", beforeCommitID, afterCommitID)
657+
actualBeforeCommitID := beforeCommitID
658+
if len(actualBeforeCommitID) == 0 {
659+
parentCommit, _ := commit.Parent(0)
660+
actualBeforeCommitID = parentCommit.ID.String()
661+
}
662+
diffArgs := []string{"diff", "-M"}
663+
if len(whitespaceBehavior) != 0 {
664+
diffArgs = append(diffArgs, whitespaceBehavior)
665+
}
666+
diffArgs = append(diffArgs, actualBeforeCommitID)
667+
diffArgs = append(diffArgs, afterCommitID)
668+
cmd = exec.Command("git", diffArgs...)
658669
}
659670
cmd.Dir = repoPath
660671
cmd.Stderr = os.Stderr

‎options/locale/locale_de-DE.ini

+5
Original file line numberDiff line numberDiff line change
@@ -1135,6 +1135,11 @@ diff.data_not_available=Keine Diff-Daten verfügbar
11351135
diff.show_diff_stats=Diff-Statistik anzeigen
11361136
diff.show_split_view=Geteilte Ansicht
11371137
diff.show_unified_view=Gesamtansicht
1138+
diff.whitespace_button=Whitespace
1139+
diff.whitespace_show_everything=Zeige alle Änderungen
1140+
diff.whitespace_ignore_all_whitespace=Ignoriere Whitespace komplett
1141+
diff.whitespace_ignore_amount_changes=Ignoriere Änderungen von Whitespace
1142+
diff.whitespace_ignore_at_eol=Ignoriere Whitespace am Zeilenende
11381143
diff.stats_desc=<strong> %d geänderte Dateien</strong> mit <strong>%d neuen</strong> und <strong>%d gelöschten</strong> Zeilen
11391144
diff.bin=BIN
11401145
diff.view_file=Datei anzeigen

‎options/locale/locale_en-US.ini

+5
Original file line numberDiff line numberDiff line change
@@ -1149,6 +1149,11 @@ diff.data_not_available = Diff Content Not Available
11491149
diff.show_diff_stats = Show Diff Stats
11501150
diff.show_split_view = Split View
11511151
diff.show_unified_view = Unified View
1152+
diff.whitespace_button = Whitespace
1153+
diff.whitespace_show_everything = Show every change
1154+
diff.whitespace_ignore_all_whitespace = Ignore all whitespace
1155+
diff.whitespace_ignore_amount_changes = Ignore whitespace changes
1156+
diff.whitespace_ignore_at_eol = Ingore whitespace at EOL
11521157
diff.stats_desc = <strong> %d changed files</strong> with <strong>%d additions</strong> and <strong>%d deletions</strong>
11531158
diff.bin = BIN
11541159
diff.view_file = View File

‎routers/repo/middlewares.go

+11
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,14 @@ func SetDiffViewStyle(ctx *context.Context) {
5050
ctx.ServerError("ErrUpdateDiffViewStyle", err)
5151
}
5252
}
53+
54+
// SetWhitespaceBehavior set whitespace behavior as render variable
55+
func SetWhitespaceBehavior(ctx *context.Context) {
56+
whitespaceBehavior := ctx.Query("whitespace")
57+
switch whitespaceBehavior {
58+
case "ignore-all", "ignore-eol", "ignore-change":
59+
ctx.Data["WhitespaceBehavior"] = whitespaceBehavior
60+
default:
61+
ctx.Data["WhitespaceBehavior"] = ""
62+
}
63+
}

‎routers/repo/pull.go

+10-3
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,12 @@ func ViewPullFiles(ctx *context.Context) {
384384
}
385385
pull := issue.PullRequest
386386

387+
whitespaceFlags := map[string]string{
388+
"ignore-all": "-w",
389+
"ignore-change": "-b",
390+
"ignore-eol": "--ignore-space-at-eol",
391+
"": ""}
392+
387393
var (
388394
diffRepoPath string
389395
startCommitID string
@@ -449,11 +455,12 @@ func ViewPullFiles(ctx *context.Context) {
449455
ctx.Data["Reponame"] = pull.HeadRepo.Name
450456
}
451457

452-
diff, err := models.GetDiffRange(diffRepoPath,
458+
diff, err := models.GetDiffRangeWithWhitespaceBehavior(diffRepoPath,
453459
startCommitID, endCommitID, setting.Git.MaxGitDiffLines,
454-
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles)
460+
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
461+
whitespaceFlags[ctx.Data["WhitespaceBehavior"].(string)])
455462
if err != nil {
456-
ctx.ServerError("GetDiffRange", err)
463+
ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
457464
return
458465
}
459466

‎routers/routes/routes.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,7 @@ func RegisterRoutes(m *macaron.Macaron) {
674674
m.Post("/merge", reqRepoWriter, bindIgnErr(auth.MergePullRequestForm{}), repo.MergePullRequest)
675675
m.Post("/cleanup", context.RepoRef(), repo.CleanUpPullRequest)
676676
m.Group("/files", func() {
677-
m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.ViewPullFiles)
677+
m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.ViewPullFiles)
678678
m.Group("/reviews", func() {
679679
m.Post("/comments", bindIgnErr(auth.CodeCommentForm{}), repo.CreateCodeComment)
680680
m.Post("/submit", bindIgnErr(auth.SubmitReviewForm{}), repo.SubmitReview)

‎templates/repo/diff/box.tmpl

+20-1
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,31 @@
11
{{if .DiffNotAvailable}}
2+
<div class="diff-detail-box diff-box ui sticky">
3+
<div>
4+
<div class="ui right">
5+
{{if .PageIsPullFiles}}
6+
{{template "repo/diff/whitespace_dropdown" .}}
7+
{{else}}
8+
<a class="ui tiny basic toggle button" href="?style={{if .IsSplitStyle}}unified{{else}}split{{end}}">{{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}}</a>
9+
{{end}}
10+
<a class="ui tiny basic toggle button" data-target="#diff-files">{{.i18n.Tr "repo.diff.show_diff_stats"}}</a>
11+
{{if and .PageIsPullFiles $.SignedUserID}}
12+
{{template "repo/diff/new_review" .}}
13+
{{end}}
14+
</div>
15+
</div>
16+
</div>
217
<h4>{{.i18n.Tr "repo.diff.data_not_available"}}</h4>
318
{{else}}
419
<div class="diff-detail-box diff-box ui sticky">
520
<div>
621
<i class="fa fa-retweet"></i>
722
{{.i18n.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
823
<div class="ui right">
9-
<a class="ui tiny basic toggle button" href="?style={{if .IsSplitStyle}}unified{{else}}split{{end}}">{{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}}</a>
24+
{{if .PageIsPullFiles}}
25+
{{template "repo/diff/whitespace_dropdown" .}}
26+
{{else}}
27+
<a class="ui tiny basic toggle button" href="?style={{if .IsSplitStyle}}unified{{else}}split{{end}}">{{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}}</a>
28+
{{end}}
1029
<a class="ui tiny basic toggle button" data-target="#diff-files">{{.i18n.Tr "repo.diff.show_diff_stats"}}</a>
1130
{{if and .PageIsPullFiles $.SignedUserID}}
1231
{{template "repo/diff/new_review" .}}

‎templates/repo/diff/whitespace_dropdown.tmpl

+39
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<div class="ui dropdown tiny button">
2+
{{.i18n.Tr "repo.diff.whitespace_button"}}
3+
<i class="dropdown icon"></i>
4+
<div class="menu">
5+
<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=">
6+
{{ if eq .WhitespaceBehavior "" }}
7+
<i class="circle dot icon"></i>
8+
{{else}}
9+
<i class="circle outline icon"></i>
10+
{{end}}
11+
{{.i18n.Tr "repo.diff.whitespace_show_everything"}}
12+
</a>
13+
<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-all">
14+
{{ if eq .WhitespaceBehavior "ignore-all" }}
15+
<i class="circle dot icon"></i>
16+
{{else}}
17+
<i class="circle outline icon"></i>
18+
{{end}}
19+
{{.i18n.Tr "repo.diff.whitespace_ignore_all_whitespace"}}
20+
</a>
21+
<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-change">
22+
{{ if eq .WhitespaceBehavior "ignore-change" }}
23+
<i class="circle dot icon"></i>
24+
{{else}}
25+
<i class="circle outline icon"></i>
26+
{{end}}
27+
{{.i18n.Tr "repo.diff.whitespace_ignore_amount_changes"}}
28+
</a>
29+
<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-eol">
30+
{{ if eq .WhitespaceBehavior "ignore-eol" }}
31+
<i class="circle dot icon"></i>
32+
{{else}}
33+
<i class="circle outline icon"></i>
34+
{{end}}
35+
{{.i18n.Tr "repo.diff.whitespace_ignore_at_eol"}}
36+
</a>
37+
</div>
38+
</div>
39+
<a class="ui tiny basic toggle button" href="?style={{if .IsSplitStyle}}unified{{else}}split{{end}}&whitespace={{$.WhitespaceBehavior}}">{{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}}</a>

0 commit comments

Comments
 (0)