Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go wrapper around git diff-tree --raw -r -M #33369

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

McRaeAlex
Copy link
Contributor

  • Implemented calling git diff-tree
  • Ensures wrapper function is called with valid arguments
  • Parses output into go struct, using strong typing when possible

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 23, 2025
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 23, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 23, 2025
@McRaeAlex McRaeAlex marked this pull request as draft January 23, 2025 20:31
@McRaeAlex McRaeAlex marked this pull request as ready for review January 23, 2025 20:32
@McRaeAlex
Copy link
Contributor Author

Context for this PR: This was split out of a larger PR I hope to upstream to gitea for showing all files in a file tree at once rather than having a show more button at the bottom.

Screenshot 2025-01-23 at 12 36 39 PM

Note that this wouldn't change the behaviour of the files list to the right of the file tree. (Lets have that discussion in the PR which modifies the frontend)

@McRaeAlex McRaeAlex requested a review from lunny January 27, 2025 18:26
@lunny
Copy link
Member

lunny commented Jan 30, 2025

And if it's for the left tree of git comparing, I don't think the structure is enough. Maybe it should like

type TreeViewNode struct {
	Name         string          `json:"name"`
	Type         string          `json:"type"`
	Path         string          `json:"path"`
	SubModuleURL string          `json:"sub_module_url,omitempty"`
	Children     []*TreeViewNode `json:"children,omitempty"`
}

This will be introduced in https://github.com/go-gitea/gitea/pull/32721/files#diff-ab9647487f3912999527f743b0fcada9b0bf1858b95e0bb7a684cc9fa383b157

@McRaeAlex
Copy link
Contributor Author

And if it's for the left tree of git comparing, I don't think the structure is enough. Maybe it should like

type TreeViewNode struct {
	Name         string          `json:"name"`
	Type         string          `json:"type"`
	Path         string          `json:"path"`
	SubModuleURL string          `json:"sub_module_url,omitempty"`
	Children     []*TreeViewNode `json:"children,omitempty"`
}

This will be introduced in https://github.com/go-gitea/gitea/pull/32721/files#diff-ab9647487f3912999527f743b0fcada9b0bf1858b95e0bb7a684cc9fa383b157

The way I did this for allspice was the same structure as this with a function for transforming from output of git diff-tree to something more consumable by the UI.

Heres the function we are using in allspice.

type FileDiffFile struct {
	Name        string
	NameHash    string
	IsSubmodule bool
	IsBinary    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
		isBinary := file.HeadMode == git.EntryModeExec

		isViewed := false
		if fileViewedState, ok := filesViewedState[file.Path()]; ok {
			isViewed = (fileViewedState == pull_model.Viewed)
		}

		files = append(files, FileDiffFile{
			Name:        file.HeadPath,
			NameHash:    nameHash,
			IsSubmodule: isSubmodule,
			IsBinary:    isBinary,
			IsViewed:    isViewed,
			Status:      file.Status,
		})
	}

	return files
}

This allows it to be used in box.tmpl

const diffDataFiles = [{{range $i, $file := .Diff.Files}}{Name:"{{$file.Name}}",NameHash:"{{$file.NameHash}}",Type:{{$file.Type}},IsBin:{{$file.IsBin}},IsSubmodule:{{$file.IsSubmodule}},Addition:{{$file.Addition}},Deletion:{{$file.Deletion}},IsViewed:{{$file.IsViewed}}},{{end}}];
with some other minor modifications (like removing the code which relies on Added and Deleted).

@McRaeAlex McRaeAlex requested a review from lunny January 30, 2025 16:55
services/gitdiff/git_diff_tree.go Outdated Show resolved Hide resolved
services/gitdiff/git_diff_tree.go Outdated Show resolved Hide resolved
services/gitdiff/git_diff_tree.go Outdated Show resolved Hide resolved
services/gitdiff/git_diff_tree.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 2, 2025
@lunny
Copy link
Member

lunny commented Feb 2, 2025

And if it's for the left tree of git comparing, I don't think the structure is enough. Maybe it should like

type TreeViewNode struct {
	Name         string          `json:"name"`
	Type         string          `json:"type"`
	Path         string          `json:"path"`
	SubModuleURL string          `json:"sub_module_url,omitempty"`
	Children     []*TreeViewNode `json:"children,omitempty"`
}

This will be introduced in #32721 (files)

The way I did this for allspice was the same structure as this with a function for transforming from output of git diff-tree to something more consumable by the UI.

Heres the function we are using in allspice.

type FileDiffFile struct {
	Name        string
	NameHash    string
	IsSubmodule bool
	IsBinary    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
		isBinary := file.HeadMode == git.EntryModeExec

		isViewed := false
		if fileViewedState, ok := filesViewedState[file.Path()]; ok {
			isViewed = (fileViewedState == pull_model.Viewed)
		}

		files = append(files, FileDiffFile{
			Name:        file.HeadPath,
			NameHash:    nameHash,
			IsSubmodule: isSubmodule,
			IsBinary:    isBinary,
			IsViewed:    isViewed,
			Status:      file.Status,
		})
	}

	return files
}

This allows it to be used in box.tmpl

const diffDataFiles = [{{range $i, $file := .Diff.Files}}{Name:"{{$file.Name}}",NameHash:"{{$file.NameHash}}",Type:{{$file.Type}},IsBin:{{$file.IsBin}},IsSubmodule:{{$file.IsSubmodule}},Addition:{{$file.Addition}},Deletion:{{$file.Deletion}},IsViewed:{{$file.IsViewed}}},{{end}}];

with some other minor modifications (like removing the code which relies on Added and Deleted).

Since we are rewriting the logic, maybe we can consider the performance more. Perhaps we can avoid traversing twice. Using a callback in the runGitDiffTree function might be more efficient, eliminating the need to create a slice of DiffTreeRecord before transforming it into another slice.

@McRaeAlex
Copy link
Contributor Author

And if it's for the left tree of git comparing, I don't think the structure is enough. Maybe it should like

type TreeViewNode struct {
	Name         string          `json:"name"`
	Type         string          `json:"type"`
	Path         string          `json:"path"`
	SubModuleURL string          `json:"sub_module_url,omitempty"`
	Children     []*TreeViewNode `json:"children,omitempty"`
}

This will be introduced in #32721 (files)

The way I did this for allspice was the same structure as this with a function for transforming from output of git diff-tree to something more consumable by the UI.
Heres the function we are using in allspice.

type FileDiffFile struct {
	Name        string
	NameHash    string
	IsSubmodule bool
	IsBinary    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
		isBinary := file.HeadMode == git.EntryModeExec

		isViewed := false
		if fileViewedState, ok := filesViewedState[file.Path()]; ok {
			isViewed = (fileViewedState == pull_model.Viewed)
		}

		files = append(files, FileDiffFile{
			Name:        file.HeadPath,
			NameHash:    nameHash,
			IsSubmodule: isSubmodule,
			IsBinary:    isBinary,
			IsViewed:    isViewed,
			Status:      file.Status,
		})
	}

	return files
}

This allows it to be used in box.tmpl

const diffDataFiles = [{{range $i, $file := .Diff.Files}}{Name:"{{$file.Name}}",NameHash:"{{$file.NameHash}}",Type:{{$file.Type}},IsBin:{{$file.IsBin}},IsSubmodule:{{$file.IsSubmodule}},Addition:{{$file.Addition}},Deletion:{{$file.Deletion}},IsViewed:{{$file.IsViewed}}},{{end}}];

with some other minor modifications (like removing the code which relies on Added and Deleted).

Since we are rewriting the logic, maybe we can consider the performance more. Perhaps we can avoid traversing twice. Using a callback in the runGitDiffTree function might be more efficient, eliminating the need to create a slice of DiffTreeRecord before transforming it into another slice.

If we want to avoid iterating over it twice, once when transforming and once when rendering I can modify transformDiffTreeForUI to take in a single record and transform it. Then it can be used in the go template.

However I think this conversation belongs in the next PR. If during the next PR we find the performance of this PRs code unacceptable for whatever reason I am happy to come back and find ways of improving it.

@delvh
Copy link
Member

delvh commented Feb 3, 2025

Also, keep in mind that the two queries serve two different purposes:
The first one is supposed to be unpaginated, the second one is still intended to be paginated.
As such, it might even be more performant to keep them separate.

@McRaeAlex
Copy link
Contributor Author

Also, keep in mind that the two queries serve two different purposes: The first one is supposed to be unpaginated, the second one is still intended to be paginated. As such, it might even be more performant to keep them separate.

With the proposed UI changes we can completely remove this script tag

<script id="diff-data-script" type="module">
when file-only=true which will make the "show more" button faster and means we don't have to recompute the file tree on the frontend after every new page.

@lunny lunny added this to the 1.24.0 milestone Feb 3, 2025
@McRaeAlex
Copy link
Contributor Author

@lunny @wxiaoguang Are there any other changes you want to this PR?

I can allocate some time this afternoon to address the testing but will only do so if we feel confident there is nothing else we want addressed.

@lunny
Copy link
Member

lunny commented Feb 5, 2025

Since #33507 modified the test data, some tests might need to be updated as well. However, I’m not sure which ones should be changed since this update doesn’t affect any existing code. Do you plan to submit additional PRs to utilize the newly introduced code? If so, we could improve the code in the following PRs.

@McRaeAlex
Copy link
Contributor Author

McRaeAlex commented Feb 5, 2025

Since #33507 modified the test data, some tests might need to be updated as well. However, I’m not sure which ones should be changed since this update doesn’t affect any existing code. Do you plan to submit additional PRs to utilize the newly introduced code? If so, we could improve the code in the following PRs.

I will update all the tests in git_diff_tree_tests.go to use to other test fixtures (like was done in https://github.com/go-gitea/gitea/pull/33507/files). I will post this patch and another stacked PR for the UI changes this afternoon.

 * Implemented calling git diff-tree
 * Ensures wrapper function is called with valid arguments
 * Parses output into go struct, using strong typing when possible
wxiaoguang
wxiaoguang previously approved these changes Feb 6, 2025
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 6, 2025
@wxiaoguang wxiaoguang dismissed their stale review February 6, 2025 03:32

Oops, still one pending: #33369 (comment)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Feb 6, 2025
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 6, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 6, 2025
@lunny lunny enabled auto-merge (squash) February 7, 2025 00:12
@lunny lunny merged commit a1f1bcc into go-gitea:main Feb 7, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 7, 2025
@McRaeAlex McRaeAlex deleted the gitea/am/diff-tree branch February 7, 2025 01:34
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 7, 2025
* giteaofficial/main:
  refactor: decouple context from migration structs (go-gitea#33399)
  Move gitgraph from modules to services layer (go-gitea#33527)
  Add go wrapper around git diff-tree --raw -r -M (go-gitea#33369)
  [skip ci] Updated translations via Crowdin
  Update MAINTAINERS (go-gitea#33529)
  Add cropping support when modifying the user/org/repo avatar (go-gitea#33498)
  [skip ci] Updated translations via Crowdin
  Add alphabetical project sorting (go-gitea#33504)
  Refactor gitdiff test (go-gitea#33507)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants