From f92a0b68fed81128fa278e82aa0e3d49d74ffdf6 Mon Sep 17 00:00:00 2001 From: Mario Lubenka Date: Fri, 4 Oct 2019 21:58:54 +0200 Subject: [PATCH] Bugfix for image compare and minor improvements to image compare (#8289) * Resolve error when comparing images Signed-off-by: Mario Lubenka * Check blob existence instead of git-ls when checking if file exists Signed-off-by: Mario Lubenka * Show file metadata also when a file was newly added Signed-off-by: Mario Lubenka * Fixes error in commit view Signed-off-by: Mario Lubenka * Excludes assigning path and image infos for compare routers to service package Signed-off-by: Mario Lubenka * Removes nil default and fixes import order Signed-off-by: Mario Lubenka * Adds missing comments Signed-off-by: Mario Lubenka * Moves methods for assigning compare data to context into repo router package Signed-off-by: Mario Lubenka * Show image compare for deleted images as well. Simplify check if image should be displayed Signed-off-by: Mario Lubenka --- modules/git/commit.go | 7 ++- routers/repo/commit.go | 24 +++------ routers/repo/compare.go | 77 ++++++++++++++++------------- routers/repo/pull.go | 25 +--------- templates/repo/diff/box.tmpl | 7 ++- templates/repo/diff/image_diff.tmpl | 57 ++++++++++++++++----- 6 files changed, 106 insertions(+), 91 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 83e03c279..eb442f988 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -355,8 +355,11 @@ func (c *Commit) FileChangedSinceCommit(filename, pastCommit string) (bool, erro // HasFile returns true if the file given exists on this commit // This does only mean it's there - it does not mean the file was changed during the commit. func (c *Commit) HasFile(filename string) (bool, error) { - result, err := c.repo.LsFiles(filename) - return result[0] == filename, err + _, err := c.GetBlobByPath(filename) + if err != nil { + return false, err + } + return true, nil } // GetSubModules get all the sub modules of current revision git tree diff --git a/routers/repo/commit.go b/routers/repo/commit.go index 919ebabf4..3cedf7031 100644 --- a/routers/repo/commit.go +++ b/routers/repo/commit.go @@ -239,24 +239,18 @@ func Diff(ctx *context.Context) { ctx.Data["CommitID"] = commitID ctx.Data["Username"] = userName ctx.Data["Reponame"] = repoName - ctx.Data["IsImageFile"] = commit.IsImageFile - ctx.Data["ImageInfo"] = func(name string) *git.ImageMetaData { - result, err := commit.ImageInfo(name) - if err != nil { - log.Error("ImageInfo failed: %v", err) - return nil - } - return result - } - ctx.Data["ImageInfoBase"] = ctx.Data["ImageInfo"] + + var parentCommit *git.Commit if commit.ParentCount() > 0 { - parentCommit, err := ctx.Repo.GitRepo.GetCommit(parents[0]) + parentCommit, err = ctx.Repo.GitRepo.GetCommit(parents[0]) if err != nil { ctx.NotFound("GetParentCommit", err) return } - ctx.Data["ImageInfo"] = parentCommit.ImageInfo } + setImageCompareContext(ctx, parentCommit, commit) + headTarget := path.Join(userName, repoName) + setPathsCompareContext(ctx, parentCommit, commit, headTarget) ctx.Data["Title"] = commit.Summary() + " ยท " + base.ShortSha(commitID) ctx.Data["Commit"] = commit ctx.Data["Verification"] = models.ParseCommitWithSignature(commit) @@ -264,8 +258,6 @@ func Diff(ctx *context.Context) { ctx.Data["Diff"] = diff ctx.Data["Parents"] = parents ctx.Data["DiffNotAvailable"] = diff.NumFiles() == 0 - ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(userName, repoName, "src", "commit", commitID) - ctx.Data["RawPath"] = setting.AppSubURL + "/" + path.Join(userName, repoName, "raw", "commit", commitID) note := &git.Note{} err = git.GetNote(ctx.Repo.GitRepo, commitID, note) @@ -275,10 +267,6 @@ func Diff(ctx *context.Context) { ctx.Data["NoteAuthor"] = models.ValidateCommitWithEmail(note.Commit) } - if commit.ParentCount() > 0 { - ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(userName, repoName, "src", "commit", parents[0]) - ctx.Data["BeforeRawPath"] = setting.AppSubURL + "/" + path.Join(userName, repoName, "raw", "commit", parents[0]) - } ctx.Data["BranchName"], err = commit.GetBranchName() if err != nil { ctx.ServerError("commit.GetBranchName", err) diff --git a/routers/repo/compare.go b/routers/repo/compare.go index 7c55dfca3..f8534f68b 100644 --- a/routers/repo/compare.go +++ b/routers/repo/compare.go @@ -5,6 +5,7 @@ package repo import ( + "fmt" "path" "strings" @@ -21,6 +22,45 @@ const ( tplCompare base.TplName = "repo/diff/compare" ) +// setPathsCompareContext sets context data for source and raw paths +func setPathsCompareContext(ctx *context.Context, base *git.Commit, head *git.Commit, headTarget string) { + sourcePath := setting.AppSubURL + "/%s/src/commit/%s" + rawPath := setting.AppSubURL + "/%s/raw/commit/%s" + + ctx.Data["SourcePath"] = fmt.Sprintf(sourcePath, headTarget, head.ID) + ctx.Data["RawPath"] = fmt.Sprintf(rawPath, headTarget, head.ID) + if base != nil { + baseTarget := path.Join(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) + ctx.Data["BeforeSourcePath"] = fmt.Sprintf(sourcePath, baseTarget, base.ID) + ctx.Data["BeforeRawPath"] = fmt.Sprintf(rawPath, baseTarget, base.ID) + } +} + +// setImageCompareContext sets context data that is required by image compare template +func setImageCompareContext(ctx *context.Context, base *git.Commit, head *git.Commit) { + ctx.Data["IsImageFileInHead"] = head.IsImageFile + ctx.Data["IsImageFileInBase"] = base.IsImageFile + ctx.Data["ImageInfoBase"] = func(name string) *git.ImageMetaData { + if base == nil { + return nil + } + result, err := base.ImageInfo(name) + if err != nil { + log.Error("ImageInfo failed: %v", err) + return nil + } + return result + } + ctx.Data["ImageInfo"] = func(name string) *git.ImageMetaData { + result, err := head.ImageInfo(name) + if err != nil { + log.Error("ImageInfo failed: %v", err) + return nil + } + return result + } +} + // ParseCompareInfo parse compare info between two commit for preparing comparing references func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *git.Repository, *git.CompareInfo, string, string) { baseRepo := ctx.Repo.Repository @@ -291,43 +331,10 @@ func PrepareCompareDiff( ctx.Data["title"] = title ctx.Data["Username"] = headUser.Name ctx.Data["Reponame"] = headRepo.Name - ctx.Data["IsImageFile"] = headCommit.IsImageFile - ctx.Data["ImageInfo"] = func(name string) *git.ImageMetaData { - result, err := headCommit.ImageInfo(name) - if err != nil { - log.Error("ImageInfo failed: %v", err) - return nil - } - return result - } - ctx.Data["FileExistsInBaseCommit"] = func(filename string) bool { - result, err := baseCommit.HasFile(filename) - if err != nil { - log.Error( - "Error while checking if file \"%s\" exists in base commit \"%s\" (repo: %s): %v", - filename, - baseCommit, - baseGitRepo.Path, - err) - return false - } - return result - } - ctx.Data["ImageInfoBase"] = func(name string) *git.ImageMetaData { - result, err := baseCommit.ImageInfo(name) - if err != nil { - log.Error("ImageInfo failed: %v", err) - return nil - } - return result - } + setImageCompareContext(ctx, baseCommit, headCommit) headTarget := path.Join(headUser.Name, repo.Name) - baseTarget := path.Join(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) - ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", headCommitID) - ctx.Data["RawPath"] = setting.AppSubURL + "/" + path.Join(headTarget, "raw", "commit", headCommitID) - ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(baseTarget, "src", "commit", baseCommitID) - ctx.Data["BeforeRawPath"] = setting.AppSubURL + "/" + path.Join(baseTarget, "raw", "commit", baseCommitID) + setPathsCompareContext(ctx, baseCommit, headCommit, headTarget) return false } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 72d2ffcaa..7af01c46b 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -564,29 +564,8 @@ func ViewPullFiles(ctx *context.Context) { return } - ctx.Data["IsImageFile"] = commit.IsImageFile - ctx.Data["ImageInfoBase"] = func(name string) *git.ImageMetaData { - result, err := baseCommit.ImageInfo(name) - if err != nil { - log.Error("ImageInfo failed: %v", err) - return nil - } - return result - } - ctx.Data["ImageInfo"] = func(name string) *git.ImageMetaData { - result, err := commit.ImageInfo(name) - if err != nil { - log.Error("ImageInfo failed: %v", err) - return nil - } - return result - } - - baseTarget := path.Join(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) - ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", endCommitID) - ctx.Data["RawPath"] = setting.AppSubURL + "/" + path.Join(headTarget, "raw", "commit", endCommitID) - ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(baseTarget, "src", "commit", startCommitID) - ctx.Data["BeforeRawPath"] = setting.AppSubURL + "/" + path.Join(baseTarget, "raw", "commit", startCommitID) + setImageCompareContext(ctx, baseCommit, commit) + setPathsCompareContext(ctx, baseCommit, commit, headTarget) ctx.Data["RequireHighlightJS"] = true ctx.Data["RequireTribute"] = true diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index edc04f906..b5fde36a6 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -106,7 +106,12 @@
{{if ne $file.Type 4}} - {{$isImage := (call $.IsImageFile $file.Name)}} + {{$isImage := false}} + {{if $file.IsDeleted}} + {{$isImage = (call $.IsImageFileInBase $file.Name)}} + {{else}} + {{$isImage = (call $.IsImageFileInHead $file.Name)}} + {{end}}
diff --git a/templates/repo/diff/image_diff.tmpl b/templates/repo/diff/image_diff.tmpl index 8fa7f6b87..6afb985e9 100644 --- a/templates/repo/diff/image_diff.tmpl +++ b/templates/repo/diff/image_diff.tmpl @@ -11,36 +11,69 @@ {{ $imageInfoBase := (call .root.ImageInfoBase .file.OldName) }} {{ $imageInfoHead := (call .root.ImageInfo .file.Name) }} -{{if and $imageInfoBase $imageInfoHead }} +{{if or $imageInfoBase $imageInfoHead }} {{end}}
- {{ $oldImageExists := (call .root.FileExistsInBaseCommit .file.OldName) }} - {{if $oldImageExists}} + {{if or .file.IsDeleted (not .file.IsCreated)}} {{end}} - - - + {{if or .file.IsCreated (not .file.IsDeleted)}} + + + + {{end}}
- {{.root.i18n.Tr "repo.diff.file_image_width"}}: {{$imageInfoBase.Width}} + {{if $imageInfoBase }} + {{ $classWidth := "" }} + {{ $classHeight := "" }} + {{ $classByteSize := "" }} + {{if $imageInfoHead}} + {{if not (eq $imageInfoBase.Width $imageInfoHead.Width)}} + {{ $classWidth = "red" }} + {{end}} + {{if not (eq $imageInfoBase.Height $imageInfoHead.Height)}} + {{ $classHeight = "red" }} + {{end}} + {{if not (eq $imageInfoBase.ByteSize $imageInfoHead.ByteSize)}} + {{ $classByteSize = "red" }} + {{end}} + {{end}} + {{.root.i18n.Tr "repo.diff.file_image_width"}}: {{$imageInfoBase.Width}}  |  - {{.root.i18n.Tr "repo.diff.file_image_height"}}: {{$imageInfoBase.Height}} + {{.root.i18n.Tr "repo.diff.file_image_height"}}: {{$imageInfoBase.Height}}  |  - {{.root.i18n.Tr "repo.diff.file_byte_size"}}: {{FileSize $imageInfoBase.ByteSize}} + {{.root.i18n.Tr "repo.diff.file_byte_size"}}: {{FileSize $imageInfoBase.ByteSize}} + {{end}} - {{.root.i18n.Tr "repo.diff.file_image_width"}}: {{$imageInfoHead.Width}} + {{if $imageInfoHead }} + {{ $classWidth := "" }} + {{ $classHeight := "" }} + {{ $classByteSize := "" }} + {{if $imageInfoBase}} + {{if not (eq $imageInfoBase.Width $imageInfoHead.Width)}} + {{ $classWidth = "green" }} + {{end}} + {{if not (eq $imageInfoBase.Height $imageInfoHead.Height)}} + {{ $classHeight = "green" }} + {{end}} + {{if not (eq $imageInfoBase.ByteSize $imageInfoHead.ByteSize)}} + {{ $classByteSize = "green" }} + {{end}} + {{end}} + {{.root.i18n.Tr "repo.diff.file_image_width"}}: {{$imageInfoHead.Width}}  |  - {{.root.i18n.Tr "repo.diff.file_image_height"}}: {{$imageInfoHead.Height}} + {{.root.i18n.Tr "repo.diff.file_image_height"}}: {{$imageInfoHead.Height}}  |  - {{.root.i18n.Tr "repo.diff.file_byte_size"}}: {{FileSize $imageInfoHead.ByteSize}} + {{.root.i18n.Tr "repo.diff.file_byte_size"}}: {{FileSize $imageInfoHead.ByteSize}} + {{end}}