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

fix: windows file tree #5364

Merged
merged 5 commits into from Nov 13, 2023
Merged

fix: windows file tree #5364

merged 5 commits into from Nov 13, 2023

Conversation

TimBeyer
Copy link
Contributor

@TimBeyer TimBeyer commented Nov 10, 2023

What this PR does / why we need it:
Ensures that the GARDEN_GIT_SCAN_MODE=repo option works correctly on windows.
That's important because on windows, spawning subprocesses can be relatively slow (see https://gregoryszorc.com/blog/2021/04/06/surprisingly-slow/) and by default we call git many times, which adds up for large projects and can really slow things down.
Previously it was broken because of assumptions around how the file system is rooted, which didn't hold under windows.
Now comes with a small test to verify the behavior.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@TimBeyer
Copy link
Contributor Author

Closing for now as on windows I would still end up with 0 files loaded when using this scan mode.

@TimBeyer
Copy link
Contributor Author

i garden               → Running in Garden environment dev.default
[silly] Scanning for configs (force=false)
[silly] Scanning for configs in C:\Users\timbe\Code\garden-large-repo-generator
[silly] TreeCache: No cached value for key ["git-repo-files","C:\\Users\\timbe\\Code\\garden-large-repo-generator"]
[silly] TreeCache: No cached value for key ["git-repo-files","C:\\Users\\timbe\\Code\\garden-large-repo-generator"]
Scanning repository at C:\Users\timbe\Code\garden-large-repo-generator
i git [debug]          → Scanning project root at C:\Users\timbe\Code\garden-large-repo-generator
  → Includes: (none)
  → Excludes: **/.garden/**/*
i git [silly]          → Calling git with args 'diff-index --name-only HEAD C:\Users\timbe\Code\garden-large-repo-generator' in C:\Users\timbe\Code\garden-large-repo-generator
i git [silly]          → Calling git with args '--glob-pathspecs ls-files --ignored --cached --exclude C:\Users\timbe\Code\garden-large-repo-generator\.garden --exclude **/.garden/**/* --exclude-per-directory .gardenignore' in C:\Users\timbe\Code\garden-large-repo-generator
i git [silly]          → Calling git with args '--glob-pathspecs ls-files -s --others --cached --exclude C:\Users\timbe\Code\garden-large-repo-generator\.garden --exclude **/.garden/**/* --exclude-per-directory .gardenignore' in C:\Users\timbe\Code\garden-large-repo-generator
i git [debug]          → Found 4430 files in project root C:\Users\timbe\Code\garden-large-repo-generator
[silly] TreeCache: Setting value for key ["git-repo-files","C:\\Users\\timbe\\Code\\garden-large-repo-generator"]
[debug] Found 4430 files in module path, filtering by 1 include and 6 exclude globs
[silly] Include globs: C:\Users\timbe\Code\garden-large-repo-generator\**\*garden.y*ml
[silly] Exclude globs: C:\Users\timbe\Code\garden-large-repo-generator\.garden\**\*, C:\Users\timbe\Code\garden-large-repo-generator\.git\**\*, C:\Users\timbe\Code\garden-large-repo-generator\.gitmodules, C:\Users\timbe\Code\garden-large-repo-generator\.garden\**\*, C:\Users\timbe\Code\garden-large-repo-generator\debug-info*\**, C:\Users\timbe\Code\garden-large-repo-generator\.garden\**\*
[debug] Found 0 files in module path after glob matching
[debug] Scanned and found 0 actions, 0 workflows and 0 modules

@TimBeyer
Copy link
Contributor Author

Comparing that with (hopefully) the same sort of setup on MacOS

ℹ garden               → Running in Garden environment dev.default
[silly] Scanning for configs (force=false)
[silly] Scanning for configs in /Users/tim/Development/garden/garden-large-repo-generator
[silly] TreeCache: No cached value for key ["git-repo-files","/Users/tim/Development/garden/garden-large-repo-generator"]
[silly] TreeCache: No cached value for key ["git-repo-files","/Users/tim/Development/garden/garden-large-repo-generator"]
Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
ℹ git [debug]          → Scanning project root at /Users/tim/Development/garden/garden-large-repo-generator
  → Includes: (none)
  → Excludes: **/.garden/**/*
ℹ git [silly]          → Calling git with args 'diff-index --name-only HEAD /Users/tim/Development/garden/garden-large-repo-generator' in /Users/tim/Development/garden/garden-large-repo-generator
ℹ git [silly]          → Calling git with args '--glob-pathspecs ls-files --ignored --cached --exclude /Users/tim/Development/garden/garden-large-repo-generator/.garden --exclude **/.garden/**/* --exclude-per-directory .gardenignore' in /Users/tim/Development/garden/garden-large-repo-generator
ℹ git [silly]          → Calling git with args '--glob-pathspecs ls-files -s --others --cached --exclude /Users/tim/Development/garden/garden-large-repo-generator/.garden --exclude **/.garden/**/* --exclude-per-directory .gardenignore' in /Users/tim/Development/garden/garden-large-repo-generator
ℹ git [debug]          → Found 5532 files in project root /Users/tim/Development/garden/garden-large-repo-generator
[silly] TreeCache: Setting value for key ["git-repo-files","/Users/tim/Development/garden/garden-large-repo-generator"]
[debug] Found 5532 files in module path, filtering by 1 include and 6 exclude globs
[silly] Include globs: /Users/tim/Development/garden/garden-large-repo-generator/**/*garden.y*ml
[silly] Exclude globs: /Users/tim/Development/garden/garden-large-repo-generator/.garden/**/*, /Users/tim/Development/garden/garden-large-repo-generator/.git/**/*, /Users/tim/Development/garden/garden-large-repo-generator/.gitmodules, /Users/tim/Development/garden/garden-large-repo-generator/.garden/**/*, /Users/tim/Development/garden/garden-large-repo-generator/debug-info*/**, /Users/tim/Development/garden/garden-large-repo-generator/.garden/**/*
[debug] Found 253 files in module path after glob matching

@TimBeyer
Copy link
Contributor Author

The issue lies here: https://github.com/micromatch/micromatch#backslashes

We join the path of the directory together with the glob to do the matching:

C:\Users\timbe\Code\garden-large-repo-generator\**\*garden.y*ml

This makes it so that the glob gets escaped and things break on windows.
We need to remove the root path from everything to match and then use globs to not match the preceding path.

@TimBeyer TimBeyer reopened this Nov 10, 2023
@TimBeyer TimBeyer marked this pull request as draft November 10, 2023 16:35
@TimBeyer
Copy link
Contributor Author

Tested on windows and found performance to be signficantly improved with this enabled.
We still need to check on other git calls that we do subsequently which also impact performance pretty negatively, but overall this is an almost 2x improvement.

Just need to look at fixing the tests in framework that relate to the glob fixes I had to do.

// In theory, node picks the right utils automatically depending on platform
// However, for testing, we need to be able to specify the platform explicitly here
// else we cannot test for example for Windows on a Unix machine.
const pathUtils = (platform ?? process.platform) === "win32" ? winPath : posixPath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaah, this is awesome! 👏

@TimBeyer TimBeyer marked this pull request as ready for review November 13, 2023 09:58
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@TimBeyer TimBeyer added this pull request to the merge queue Nov 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 13, 2023
@stefreak stefreak added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit c5c3c66 Nov 13, 2023
46 checks passed
@stefreak stefreak deleted the fix/windows-file-tree branch November 13, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants