Add whitespace handling to PR-comparsion (#4683)

* 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 <felix@nehrke.info>

models/git_diff.go

@@ -633,6 +633,13 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
 // passing the empty string as beforeCommitID returns a diff from the
 // parent commit.
 func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int) (*Diff, error) {
+	return GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID, maxLines, maxLineCharacters, maxFiles, "")
+}
+
+// GetDiffRangeWithWhitespaceBehavior builds a Diff between two commits of a repository.
+// Passing the empty string as beforeCommitID returns a diff from the parent commit.
+// The whitespaceBehavior is either an empty string or a git flag
+func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string) (*Diff, error) {
 	gitRepo, err := git.OpenRepository(repoPath)
 	if err != nil {
 		return nil, err
@@ -644,17 +651,21 @@ func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxL
 	}
 
 	var cmd *exec.Cmd
-	// if "after" commit given
-	if len(beforeCommitID) == 0 {
-		// First commit of repository.
-		if commit.ParentCount() == 0 {
+	if len(beforeCommitID) == 0 && commit.ParentCount() == 0 {
 		cmd = exec.Command("git", "show", afterCommitID)
 	} else {
-			c, _ := commit.Parent(0)
-			cmd = exec.Command("git", "diff", "-M", c.ID.String(), afterCommitID)
-		}
-	} else {
-		cmd = exec.Command("git", "diff", "-M", beforeCommitID, afterCommitID)
+		actualBeforeCommitID := beforeCommitID
+		if len(actualBeforeCommitID) == 0 {
+			parentCommit, _ := commit.Parent(0)
+			actualBeforeCommitID = parentCommit.ID.String()
+		}
+		diffArgs := []string{"diff", "-M"}
+		if len(whitespaceBehavior) != 0 {
+			diffArgs = append(diffArgs, whitespaceBehavior)
+		}
+		diffArgs = append(diffArgs, actualBeforeCommitID)
+		diffArgs = append(diffArgs, afterCommitID)
+		cmd = exec.Command("git", diffArgs...)
 	}
 	cmd.Dir = repoPath
 	cmd.Stderr = os.Stderr

options/locale/locale_en-US.ini

@@ -1152,6 +1152,11 @@ diff.data_not_available = Diff Content Not Available
 diff.show_diff_stats = Show Diff Stats
 diff.show_split_view = Split View
 diff.show_unified_view = Unified View
+diff.whitespace_button = Whitespace
+diff.whitespace_show_everything = Show all changes
+diff.whitespace_ignore_all_whitespace = Ignore whitespace when comparing lines
+diff.whitespace_ignore_amount_changes = Ignore changes in amount of whitespace
+diff.whitespace_ignore_at_eol = Ignore changes in whitespace at EOL
 diff.stats_desc = <strong> %d changed files</strong> with <strong>%d additions</strong> and <strong>%d deletions</strong>
 diff.bin = BIN
 diff.view_file = View File

routers/repo/middlewares.go

@@ -50,3 +50,14 @@ func SetDiffViewStyle(ctx *context.Context) {
 		ctx.ServerError("ErrUpdateDiffViewStyle", err)
 	}
 }
+
+// SetWhitespaceBehavior set whitespace behavior as render variable
+func SetWhitespaceBehavior(ctx *context.Context) {
+	whitespaceBehavior := ctx.Query("whitespace")
+	switch whitespaceBehavior {
+	case "ignore-all", "ignore-eol", "ignore-change":
+		ctx.Data["WhitespaceBehavior"] = whitespaceBehavior
+	default:
+		ctx.Data["WhitespaceBehavior"] = ""
+	}
+}

routers/repo/pull.go

@@ -390,6 +390,12 @@ func ViewPullFiles(ctx *context.Context) {
 	}
 	pull := issue.PullRequest
 
+	whitespaceFlags := map[string]string{
+		"ignore-all":    "-w",
+		"ignore-change": "-b",
+		"ignore-eol":    "--ignore-space-at-eol",
+		"":              ""}
+
 	var (
 		diffRepoPath  string
 		startCommitID string
@@ -455,11 +461,12 @@ func ViewPullFiles(ctx *context.Context) {
 		ctx.Data["Reponame"] = pull.HeadRepo.Name
 	}
 
-	diff, err := models.GetDiffRange(diffRepoPath,
+	diff, err := models.GetDiffRangeWithWhitespaceBehavior(diffRepoPath,
 		startCommitID, endCommitID, setting.Git.MaxGitDiffLines,
-		setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles)
+		setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
+		whitespaceFlags[ctx.Data["WhitespaceBehavior"].(string)])
 	if err != nil {
-		ctx.ServerError("GetDiffRange", err)
+		ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
 		return
 	}
 

routers/routes/routes.go

@@ -674,7 +674,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Post("/merge", reqRepoWriter, bindIgnErr(auth.MergePullRequestForm{}), repo.MergePullRequest)
 			m.Post("/cleanup", context.RepoRef(), repo.CleanUpPullRequest)
 			m.Group("/files", func() {
-				m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.ViewPullFiles)
+				m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.ViewPullFiles)
 				m.Group("/reviews", func() {
 					m.Post("/comments", bindIgnErr(auth.CodeCommentForm{}), repo.CreateCodeComment)
 					m.Post("/submit", bindIgnErr(auth.SubmitReviewForm{}), repo.SubmitReview)

templates/repo/diff/box.tmpl

@@ -1,4 +1,19 @@
 {{if .DiffNotAvailable}}
+	<div class="diff-detail-box diff-box ui sticky">
+		<div>
+			<div class="ui right">
+				{{if .PageIsPullFiles}}
+					{{template "repo/diff/whitespace_dropdown" .}}
+				{{else}}
+					<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>
+				{{end}}
+				<a class="ui tiny basic toggle button" data-target="#diff-files">{{.i18n.Tr "repo.diff.show_diff_stats"}}</a>
+				{{if and .PageIsPullFiles $.SignedUserID}}
+					{{template "repo/diff/new_review" .}}
+				{{end}}
+			</div>
+		</div>
+	</div>
 	<h4>{{.i18n.Tr "repo.diff.data_not_available"}}</h4>
 {{else}}
 	<div class="diff-detail-box diff-box ui sticky">
@@ -6,7 +21,11 @@
 			<i class="fa fa-retweet"></i>
 			{{.i18n.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
 			<div class="ui right">
+				{{if .PageIsPullFiles}}
+					{{template "repo/diff/whitespace_dropdown" .}}
+				{{else}}
 					<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>
+				{{end}}
 				<a class="ui tiny basic toggle button" data-target="#diff-files">{{.i18n.Tr "repo.diff.show_diff_stats"}}</a>
 				{{if and .PageIsPullFiles $.SignedUserID}}
 					{{template "repo/diff/new_review" .}}

templates/repo/diff/whitespace_dropdown.tmpl

@@ -0,0 +1,23 @@
+<div class="ui dropdown tiny button">
+	{{.i18n.Tr "repo.diff.whitespace_button"}}
+	<i class="dropdown icon"></i>
+	<div class="menu">
+		<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=">
+			<i class="circle {{ if eq .WhitespaceBehavior "" }}dot{{else}}outline{{end}} icon"></i>
+			{{.i18n.Tr "repo.diff.whitespace_show_everything"}}
+		</a>
+		<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-all">
+			<i class="circle {{ if eq .WhitespaceBehavior "ignore-all" }}dot{{else}}outline{{end}} icon"></i>
+			{{.i18n.Tr "repo.diff.whitespace_ignore_all_whitespace"}}
+		</a>
+		<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-change">
+			<i class="circle {{ if eq .WhitespaceBehavior "ignore-change" }}dot{{else}}outline{{end}} icon"></i>
+			{{.i18n.Tr "repo.diff.whitespace_ignore_amount_changes"}}
+		</a>
+		<a class="item" href="?style={{if .IsSplitStyle}}split{{else}}unified{{end}}&whitespace=ignore-eol">
+			<i class="circle {{ if eq .WhitespaceBehavior "ignore-eol" }}dot{{else}}outline{{end}} icon"></i>
+			{{.i18n.Tr "repo.diff.whitespace_ignore_at_eol"}}
+		</a>
+	</div>
+</div>
+<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>