fix(vcs): overflow error when repo contains large number of files #1165
Conversation
|
Does the change anything for #1097? |
|
No, this doesn't address that issue. |
|
|
||
| // We push to the output array when all ls-files commands "agree" that it should be included, | ||
| // and it passes through the include/exclude filters. | ||
| if (paths[resolvedPath] === this.ignoreFiles.length && matchPath(filePath, include, exclude)) { |
There was a problem hiding this comment.
Could you help me understand this: paths[resolvedPath] === this.ignoreFiles.length? (It's early Monday morning, so I'm a little slow.)
There was a problem hiding this comment.
NVM, I get it. The control flow is a little hard to follow but I'm not sure if there's a way to make it simpler without sacrificing perf.
| } | ||
| } | ||
|
|
||
| // We run ls-files for each ignore file and collect each return result line with `handleLine` |
There was a problem hiding this comment.
Could you add to the comment that this is the function that actually triggers the population of the output array? It's a little confusing because we're mapping over this.ignoredFiles to get the actual files (since that's how the git command works).
| // if we get 128 we're not in a repo root, so we get no files | ||
| if (err.exitCode !== 128) { | ||
| throw err | ||
| // List modified files, so that we can ensure we have the right hash for them later |
There was a problem hiding this comment.
It might be useful to document the sequencing above the function since the control flow is a little hard to follow.
There was a problem hiding this comment.
Regardless, we should at least add a comment saying that this function returns files filtered against ignore files, includes and excludes, since the name is pretty generic.
garden-service/src/vcs/git.ts
Outdated
| // We run ls-files for each ignoreFile and do a manual set-intersection (by counting elements in an object) | ||
| // in order to optimize the flow. | ||
| const paths: { [path: string]: number } = {} | ||
| const output: VcsFile[] = [] |
There was a problem hiding this comment.
Rename to files or similar?
We now run fewer git commands to list files in a directory, and avoid having to list every file before later filtering out ignored files. We also use streaming to avoid any buffering issues, and for improved efficiency. Fixes #1162
edvald
force-pushed
the
fix-large-getfiles
branch
from
5d71306 to
4f8a8a8
Compare
What this PR does / why we need it:
We now run fewer git commands to list files in a directory, and avoid
having to list every file before later filtering out ignored files.
We also use streaming to avoid any buffering issues, and for improved
efficiency.
Which issue(s) this PR fixes:
Fixes #1162