From aba96f65cd687868c6eff5a079cfc7b25740642d Mon Sep 17 00:00:00 2001 From: Alexander McRae Date: Thu, 27 Feb 2025 16:58:25 -0800 Subject: [PATCH] Use `git diff-tree` for `DiffFileTree` on diff pages (#33514) Modify Diff View FileTree to show all files ## Changes * removes Show Status button on diff * uses `git diff-tree` to generate the file tree for the diff * doesn't reload the diff tree each time we load more files in the preview * selecting and unloaded file will keep loading until that file is loaded * removes `DiffFileList.vue` and "Show Stats" in diff options ## Open Questions * selecting and unloaded file will keep loading until that file is loaded. Is this behaviour okay? It matches what github does. ### Demo In this demo I set `git.MAX_GIT_DIFF_FILES=1` in my `app.ini` to demonstrate a worst case example. In most cases the behaviour isn't nearly as jarring as we load a bunch of files at a time. https://github.com/user-attachments/assets/72f29663-d6fc-472d-94fa-7fb5950c2836 --------- Co-authored-by: silverwind Co-authored-by: wxiaoguang --- options/locale/locale_en-US.ini | 1 - routers/web/repo/commit.go | 12 +++ routers/web/repo/compare.go | 10 +++ routers/web/repo/pull.go | 23 ++++++ routers/web/repo/treelist.go | 32 ++++++++ templates/repo/diff/box.tmpl | 26 ------- templates/repo/diff/options_dropdown.tmpl | 1 - web_src/js/components/DiffFileList.vue | 60 --------------- web_src/js/components/DiffFileTree.vue | 72 +----------------- web_src/js/components/DiffFileTreeItem.vue | 67 ++++++++--------- web_src/js/features/repo-diff-filetree.ts | 9 --- web_src/js/features/repo-diff.ts | 69 +++++++++++------ web_src/js/modules/stores.ts | 9 ++- web_src/js/utils.test.ts | 8 +- web_src/js/utils.ts | 6 ++ web_src/js/utils/filetree.test.ts | 86 ++++++++++++++++++++++ web_src/js/utils/filetree.ts | 85 +++++++++++++++++++++ 17 files changed, 352 insertions(+), 224 deletions(-) delete mode 100644 web_src/js/components/DiffFileList.vue create mode 100644 web_src/js/utils/filetree.test.ts create mode 100644 web_src/js/utils/filetree.ts diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 5aa6d19dae..1da2fb61ad 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2594,7 +2594,6 @@ diff.commit = commit diff.git-notes = Notes diff.data_not_available = Diff Content Not Available diff.options_button = Diff Options -diff.show_diff_stats = Show Stats diff.download_patch = Download Patch File diff.download_diff = Download Diff File diff.show_split_view = Split View diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 9c12ef9297..2728eda8b6 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -344,18 +344,30 @@ func Diff(ctx *context.Context) { ctx.Data["Reponame"] = repoName var parentCommit *git.Commit + var parentCommitID string if commit.ParentCount() > 0 { parentCommit, err = gitRepo.GetCommit(parents[0]) if err != nil { ctx.NotFound(err) return } + parentCommitID = parentCommit.ID.String() } setCompareContext(ctx, parentCommit, commit, userName, repoName) ctx.Data["Title"] = commit.Summary() + " ยท " + base.ShortSha(commitID) ctx.Data["Commit"] = commit ctx.Data["Diff"] = diff + if !fileOnly { + diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, parentCommitID, commitID) + if err != nil { + ctx.ServerError("GetDiffTree", err) + return + } + + ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, nil) + } + statuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, commitID, db.ListOptionsAll) if err != nil { log.Error("GetLatestCommitStatus: %v", err) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 71bce759a9..5165636a52 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -633,6 +633,16 @@ func PrepareCompareDiff( ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0 + if !fileOnly { + diffTree, err := gitdiff.GetDiffTree(ctx, ci.HeadGitRepo, false, beforeCommitID, headCommitID) + if err != nil { + ctx.ServerError("GetDiffTree", err) + return false + } + + ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, nil) + } + headCommit, err := ci.HeadGitRepo.GetCommit(headCommitID) if err != nil { ctx.ServerError("GetCommit", err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 223f8d017e..0769f456ec 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -761,6 +761,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi var methodWithError string var diff *gitdiff.Diff + shouldGetUserSpecificDiff := false // if we're not logged in or only a single commit (or commit range) is shown we // have to load only the diff and not get the viewed information @@ -772,6 +773,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } else { diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...) methodWithError = "SyncAndGetUserSpecificDiff" + shouldGetUserSpecificDiff = true } if err != nil { ctx.ServerError(methodWithError, err) @@ -816,6 +818,27 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } } + if !fileOnly { + // note: use mergeBase is set to false because we already have the merge base from the pull request info + diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, pull.MergeBase, headCommitID) + if err != nil { + ctx.ServerError("GetDiffTree", err) + return + } + + filesViewedState := make(map[string]pull_model.ViewedState) + if shouldGetUserSpecificDiff { + // This sort of sucks because we already fetch this when getting the diff + review, err := pull_model.GetNewestReviewState(ctx, ctx.Doer.ID, issue.ID) + if err == nil && review != nil && review.UpdatedFiles != nil { + // If there wasn't an error and we have a review with updated files, use that + filesViewedState = review.UpdatedFiles + } + } + + ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, filesViewedState) + } + ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0 diff --git a/routers/web/repo/treelist.go b/routers/web/repo/treelist.go index d11af4669f..9ce9f8424d 100644 --- a/routers/web/repo/treelist.go +++ b/routers/web/repo/treelist.go @@ -6,9 +6,11 @@ package repo import ( "net/http" + pull_model "code.gitea.io/gitea/models/pull" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/gitdiff" "github.com/go-enry/go-enry/v2" ) @@ -52,3 +54,33 @@ func isExcludedEntry(entry *git.TreeEntry) bool { return false } + +type FileDiffFile struct { + Name string + NameHash string + IsSubmodule bool + IsViewed bool + Status string +} + +// transformDiffTreeForUI transforms a DiffTree into a slice of FileDiffFile for UI rendering +// it also takes a map of file names to their viewed state, which is used to mark files as viewed +func transformDiffTreeForUI(diffTree *gitdiff.DiffTree, filesViewedState map[string]pull_model.ViewedState) []FileDiffFile { + files := make([]FileDiffFile, 0, len(diffTree.Files)) + + for _, file := range diffTree.Files { + nameHash := git.HashFilePathForWebUI(file.HeadPath) + isSubmodule := file.HeadMode == git.EntryModeCommit + isViewed := filesViewedState[file.HeadPath] == pull_model.Viewed + + files = append(files, FileDiffFile{ + Name: file.HeadPath, + NameHash: nameHash, + IsSubmodule: isSubmodule, + IsViewed: isViewed, + Status: file.Status, + }) + } + + return files +} diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index a3b64b8a11..8de54b932a 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -57,32 +57,6 @@
{{ctx.Locale.Tr "repo.pulls.showing_specified_commit_range" (ShortSha .BeforeCommitID) (ShortSha .AfterCommitID)}} - {{ctx.Locale.Tr "repo.pulls.show_all_commits"}}
{{end}} - -
{{end}}
{{if $showFileTree}} diff --git a/templates/repo/diff/options_dropdown.tmpl b/templates/repo/diff/options_dropdown.tmpl index 09b7b80e41..8d08e7ad46 100644 --- a/templates/repo/diff/options_dropdown.tmpl +++ b/templates/repo/diff/options_dropdown.tmpl @@ -1,7 +1,6 @@
{{svg "octicon-kebab-horizontal"}}
- {{ctx.Locale.Tr "repo.diff.show_diff_stats"}} {{if .Issue.Index}} {{ctx.Locale.Tr "repo.diff.download_patch"}} {{ctx.Locale.Tr "repo.diff.download_diff"}} diff --git a/web_src/js/components/DiffFileList.vue b/web_src/js/components/DiffFileList.vue deleted file mode 100644 index 6570c92781..0000000000 --- a/web_src/js/components/DiffFileList.vue +++ /dev/null @@ -1,60 +0,0 @@ - - - diff --git a/web_src/js/components/DiffFileTree.vue b/web_src/js/components/DiffFileTree.vue index d00d03565f..381a1c3ca4 100644 --- a/web_src/js/components/DiffFileTree.vue +++ b/web_src/js/components/DiffFileTree.vue @@ -1,75 +1,18 @@ diff --git a/web_src/js/components/DiffFileTreeItem.vue b/web_src/js/components/DiffFileTreeItem.vue index d3be10e3e9..5ee0e5bcaa 100644 --- a/web_src/js/components/DiffFileTreeItem.vue +++ b/web_src/js/components/DiffFileTreeItem.vue @@ -2,21 +2,7 @@ import {SvgIcon, type SvgName} from '../svg.ts'; import {diffTreeStore} from '../modules/stores.ts'; import {ref} from 'vue'; - -type File = { - Name: string; - NameHash: string; - Type: number; - IsViewed: boolean; - IsSubmodule: boolean; -} - -export type Item = { - name: string; - isFile: boolean; - file?: File; - children?: Item[]; -}; +import type {Item, File, FileStatus} from '../utils/filetree.ts'; defineProps<{ item: Item, @@ -25,15 +11,16 @@ defineProps<{ const store = diffTreeStore(); const collapsed = ref(false); -function getIconForDiffType(pType: number) { - const diffTypes: Record}> = { - '1': {name: 'octicon-diff-added', classes: ['text', 'green']}, - '2': {name: 'octicon-diff-modified', classes: ['text', 'yellow']}, - '3': {name: 'octicon-diff-removed', classes: ['text', 'red']}, - '4': {name: 'octicon-diff-renamed', classes: ['text', 'teal']}, - '5': {name: 'octicon-diff-renamed', classes: ['text', 'green']}, // there is no octicon for copied, so renamed should be ok +function getIconForDiffStatus(pType: FileStatus) { + const diffTypes: Record }> = { + 'added': {name: 'octicon-diff-added', classes: ['text', 'green']}, + 'modified': {name: 'octicon-diff-modified', classes: ['text', 'yellow']}, + 'deleted': {name: 'octicon-diff-removed', classes: ['text', 'red']}, + 'renamed': {name: 'octicon-diff-renamed', classes: ['text', 'teal']}, + 'copied': {name: 'octicon-diff-renamed', classes: ['text', 'green']}, + 'typechange': {name: 'octicon-diff-modified', classes: ['text', 'green']}, // there is no octicon for copied, so renamed should be ok }; - return diffTypes[String(pType)]; + return diffTypes[pType]; } function fileIcon(file: File) { @@ -48,27 +35,37 @@ function fileIcon(file: File) { {{ item.name }} - + -
- - - - {{ item.name }} -
-
- -
+