From ca112f0a04ea7f4fdb8e6dc1e83e293a598abc50 Mon Sep 17 00:00:00 2001 From: nemoinho Date: Tue, 14 Aug 2018 19:49:33 +0200 Subject: [PATCH] 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 --- models/git_diff.go | 31 +++++++++++++------- options/locale/locale_en-US.ini | 5 ++++ routers/repo/middlewares.go | 11 +++++++ routers/repo/pull.go | 13 ++++++-- routers/routes/routes.go | 2 +- templates/repo/diff/box.tmpl | 21 ++++++++++++- templates/repo/diff/whitespace_dropdown.tmpl | 23 +++++++++++++++ 7 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 templates/repo/diff/whitespace_dropdown.tmpl diff --git a/models/git_diff.go b/models/git_diff.go index 006238cd0..d288ea50b 100644 --- a/models/git_diff.go +++ b/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 { - cmd = exec.Command("git", "show", afterCommitID) - } else { - c, _ := commit.Parent(0) - cmd = exec.Command("git", "diff", "-M", c.ID.String(), afterCommitID) - } + if len(beforeCommitID) == 0 && commit.ParentCount() == 0 { + cmd = exec.Command("git", "show", 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 diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 2b0478748..635ddd66b 100644 --- a/options/locale/locale_en-US.ini +++ b/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 = %d changed files with %d additions and %d deletions diff.bin = BIN diff.view_file = View File diff --git a/routers/repo/middlewares.go b/routers/repo/middlewares.go index 8afad5be6..4dee272ed 100644 --- a/routers/repo/middlewares.go +++ b/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"] = "" + } +} diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 6a1aaf314..57fe33f85 100644 --- a/routers/repo/pull.go +++ b/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 } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index cc36829ef..e2448a744 100644 --- a/routers/routes/routes.go +++ b/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) diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 0a9552acc..382442fe3 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -1,4 +1,19 @@ {{if .DiffNotAvailable}} +
+
+
+ {{if .PageIsPullFiles}} + {{template "repo/diff/whitespace_dropdown" .}} + {{else}} + {{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}} + {{end}} + {{.i18n.Tr "repo.diff.show_diff_stats"}} + {{if and .PageIsPullFiles $.SignedUserID}} + {{template "repo/diff/new_review" .}} + {{end}} +
+
+

{{.i18n.Tr "repo.diff.data_not_available"}}

{{else}}
@@ -6,7 +21,11 @@ {{.i18n.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
- {{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}} + {{if .PageIsPullFiles}} + {{template "repo/diff/whitespace_dropdown" .}} + {{else}} + {{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}} + {{end}} {{.i18n.Tr "repo.diff.show_diff_stats"}} {{if and .PageIsPullFiles $.SignedUserID}} {{template "repo/diff/new_review" .}} diff --git a/templates/repo/diff/whitespace_dropdown.tmpl b/templates/repo/diff/whitespace_dropdown.tmpl new file mode 100644 index 000000000..65fd871ba --- /dev/null +++ b/templates/repo/diff/whitespace_dropdown.tmpl @@ -0,0 +1,23 @@ +
+ {{.i18n.Tr "repo.diff.whitespace_button"}} + +
+ + + {{.i18n.Tr "repo.diff.whitespace_show_everything"}} + + + + {{.i18n.Tr "repo.diff.whitespace_ignore_all_whitespace"}} + + + + {{.i18n.Tr "repo.diff.whitespace_ignore_amount_changes"}} + + + + {{.i18n.Tr "repo.diff.whitespace_ignore_at_eol"}} + +
+
+{{ if .IsSplitStyle }}{{.i18n.Tr "repo.diff.show_unified_view"}}{{else}}{{.i18n.Tr "repo.diff.show_split_view"}}{{end}}