git: worktree, optimize infiles function for very large repos#1852
git: worktree, optimize infiles function for very large repos#1852pjbgf merged 3 commits intogo-git:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes Worktree reset filtering to avoid the O(N²) filepath.Clean behavior reported in go-git issue #1845 during clone/checkout paths (notably MergeReset).
Changes:
- Replaces repeated linear scans over
opts.Fileswith a pre-cleaned lookup map for O(1) membership checks. - Updates
inFilesto operate on the precomputed set instead of repeatedly cleaning and scanning a slice. - Applies the optimization to both
resetIndexandresetWorktreefiltering paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
worktree.go
Outdated
| filesMap := make(map[string]bool, len(files)) | ||
| for _, f := range files { | ||
| filesMap[filepath.Clean(f)] = true | ||
| } |
There was a problem hiding this comment.
The logic to build a cleaned set of files is duplicated here and in resetWorktree. Consider extracting a small helper (e.g., build a cleaned file-set) to keep the two call sites consistent and reduce the chance of future regressions in this hot path.
There was a problem hiding this comment.
This is not required. These are simple conversions and meaningful util function can not be extracted.
EladGabay
left a comment
EladGabay
left a comment
There was a problem hiding this comment.
minor suggestion: map[string]struct{} is more idiomatic than map[string]any for set semantics, avoids allocating an interface header per entry
worktree.go
Outdated
| } | ||
|
|
||
| removedFiles := make([]string, 0, len(changes)) | ||
| var filesMap map[string]any |
There was a problem hiding this comment.
| var filesMap map[string]any | |
| var filesMap map[string]struct{} |
worktree.go
Outdated
| var filesMap map[string]any | ||
| if len(files) > 0 { | ||
| filesMap = make(map[string]any, len(files)) | ||
| for _, f := range files { | ||
| filesMap[filepath.Clean(f)] = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's extract this into a helper func instead, to avoid defining this twice.
worktree.go
Outdated
| removedFiles := make([]string, 0, len(changes)) | ||
| var filesMap map[string]any | ||
| if len(files) > 0 { | ||
| filesMap = make(map[string]any, len(files)) |
There was a problem hiding this comment.
| filesMap = make(map[string]any, len(files)) | |
| filesMap = make(map[string]struct{}, len(files)) |
worktree.go
Outdated
| if len(files) > 0 { | ||
| filesMap = make(map[string]any, len(files)) | ||
| for _, f := range files { | ||
| filesMap[filepath.Clean(f)] = nil |
There was a problem hiding this comment.
| filesMap[filepath.Clean(f)] = nil | |
| filesMap[filepath.Clean(f)] = struct{}{} |
|
Thank you for the review @pjbgf and @EladGabay I have addressed the review comments. Could you please take a look again ? |
Relates to #1845.