From 68df14bdad8dceea376cd4b2c2cf631793e95eb2 Mon Sep 17 00:00:00 2001 From: Felix Nehrke Date: Sat, 11 Aug 2018 20:44:55 +0200 Subject: [PATCH 1/4] 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_de-DE.ini | 5 +++ 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 | 39 ++++++++++++++++++++ 8 files changed, 112 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 006238cd0617..d288ea50b115 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_de-DE.ini b/options/locale/locale_de-DE.ini index 0f89c4c7fdde..07d32ccfd7ba 100644 --- a/options/locale/locale_de-DE.ini +++ b/options/locale/locale_de-DE.ini @@ -1135,6 +1135,11 @@ diff.data_not_available=Keine Diff-Daten verfügbar diff.show_diff_stats=Diff-Statistik anzeigen diff.show_split_view=Geteilte Ansicht diff.show_unified_view=Gesamtansicht +diff.whitespace_button=Whitespace +diff.whitespace_show_everything=Zeige alle Änderungen +diff.whitespace_ignore_all_whitespace=Ignoriere Whitespace komplett +diff.whitespace_ignore_amount_changes=Ignoriere Änderungen von Whitespace +diff.whitespace_ignore_at_eol=Ignoriere Whitespace am Zeilenende diff.stats_desc= %d geänderte Dateien mit %d neuen und %d gelöschten Zeilen diff.bin=BIN diff.view_file=Datei anzeigen diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index d39f601a4bfb..9ec6a8e243f7 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1149,6 +1149,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 every change +diff.whitespace_ignore_all_whitespace = Ignore all whitespace +diff.whitespace_ignore_amount_changes = Ignore whitespace changes +diff.whitespace_ignore_at_eol = Ingore 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 8afad5be64d1..4dee272edb51 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 e6592e1f5752..55961421f44f 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -384,6 +384,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 @@ -449,11 +455,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 cc36829ef551..e2448a74467c 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 0a9552acc1ee..382442fe3471 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 000000000000..6d00a0826fe1 --- /dev/null +++ b/templates/repo/diff/whitespace_dropdown.tmpl @@ -0,0 +1,39 @@ +
+ {{.i18n.Tr "repo.diff.whitespace_button"}} + +
+ + {{ if eq .WhitespaceBehavior "" }} + + {{else}} + + {{end}} + {{.i18n.Tr "repo.diff.whitespace_show_everything"}} + + + {{ if eq .WhitespaceBehavior "ignore-all" }} + + {{else}} + + {{end}} + {{.i18n.Tr "repo.diff.whitespace_ignore_all_whitespace"}} + + + {{ if eq .WhitespaceBehavior "ignore-change" }} + + {{else}} + + {{end}} + {{.i18n.Tr "repo.diff.whitespace_ignore_amount_changes"}} + + + {{ if eq .WhitespaceBehavior "ignore-eol" }} + + {{else}} + + {{end}} + {{.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}} From be4c6c39e5a76ed332359291756eb5ef9abd62f9 Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Mon, 13 Aug 2018 21:01:53 +0300 Subject: [PATCH 2/4] Remove de-DE translation --- options/locale/locale_de-DE.ini | 5 ----- 1 file changed, 5 deletions(-) diff --git a/options/locale/locale_de-DE.ini b/options/locale/locale_de-DE.ini index 07d32ccfd7ba..0f89c4c7fdde 100644 --- a/options/locale/locale_de-DE.ini +++ b/options/locale/locale_de-DE.ini @@ -1135,11 +1135,6 @@ diff.data_not_available=Keine Diff-Daten verfügbar diff.show_diff_stats=Diff-Statistik anzeigen diff.show_split_view=Geteilte Ansicht diff.show_unified_view=Gesamtansicht -diff.whitespace_button=Whitespace -diff.whitespace_show_everything=Zeige alle Änderungen -diff.whitespace_ignore_all_whitespace=Ignoriere Whitespace komplett -diff.whitespace_ignore_amount_changes=Ignoriere Änderungen von Whitespace -diff.whitespace_ignore_at_eol=Ignoriere Whitespace am Zeilenende diff.stats_desc= %d geänderte Dateien mit %d neuen und %d gelöschten Zeilen diff.bin=BIN diff.view_file=Datei anzeigen From b4c5d8553482b0d5f5f466fb8600c9e995e2267e Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Mon, 13 Aug 2018 21:20:09 +0300 Subject: [PATCH 3/4] Update whitespace option text --- options/locale/locale_en-US.ini | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 9ec6a8e243f7..4012403b79f8 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1150,10 +1150,10 @@ 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 every change -diff.whitespace_ignore_all_whitespace = Ignore all whitespace -diff.whitespace_ignore_amount_changes = Ignore whitespace changes -diff.whitespace_ignore_at_eol = Ingore whitespace at EOL +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 From d8f7767b673f143ebf9775d094c2b4154a746e1b Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Mon, 13 Aug 2018 21:38:19 +0300 Subject: [PATCH 4/4] Optimize dropdown template code to be less verbose --- templates/repo/diff/whitespace_dropdown.tmpl | 24 ++++---------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/templates/repo/diff/whitespace_dropdown.tmpl b/templates/repo/diff/whitespace_dropdown.tmpl index 6d00a0826fe1..65fd871ba5af 100644 --- a/templates/repo/diff/whitespace_dropdown.tmpl +++ b/templates/repo/diff/whitespace_dropdown.tmpl @@ -3,35 +3,19 @@
- {{ if eq .WhitespaceBehavior "" }} - - {{else}} - - {{end}} + {{.i18n.Tr "repo.diff.whitespace_show_everything"}} - {{ if eq .WhitespaceBehavior "ignore-all" }} - - {{else}} - - {{end}} + {{.i18n.Tr "repo.diff.whitespace_ignore_all_whitespace"}} - {{ if eq .WhitespaceBehavior "ignore-change" }} - - {{else}} - - {{end}} + {{.i18n.Tr "repo.diff.whitespace_ignore_amount_changes"}} - {{ if eq .WhitespaceBehavior "ignore-eol" }} - - {{else}} - - {{end}} + {{.i18n.Tr "repo.diff.whitespace_ignore_at_eol"}}