From e31fe4d5eeb890bae4e1444f9da6af971977a89b Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Thu, 13 Jul 2023 04:21:46 +0200 Subject: [PATCH] perf(git): optimize git scan when exclude but no include filter is set This can be a major optimization in certain cases. The reason this wasn't done previously was due to a git ls-files issue but we missed out on this path when no includes are used, in which case the issue doesn't come up. Usually includes are much more specific than excludes in configs, so this approach should cover a lot of user scenarios. It is still more optimal to use `.gardenignore` files over exclude patterns where possible, but using excludes in configs is obviously a common and supported use case, and not always possible to use the ignore files (say, if many actions/modules share a directory and need different exclusions). --- core/src/vcs/git.ts | 43 ++++++++++++++++++++++++++++------- core/test/unit/src/vcs/git.ts | 2 +- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index a2d1474c1f..c8d8b1c1a4 100644 --- a/core/src/vcs/git.ts +++ b/core/src/vcs/git.ts @@ -300,17 +300,40 @@ export class GitHandler extends VcsHandler { const absExcludes = exclude.map((p) => resolve(path, p)) - // Apply the include patterns to the ls-files queries. We use the 'glob' "magic word" (in git parlance) + // Apply the include patterns to the ls-files queries. We use the --glob-pathspecs flag // to make sure the path handling is consistent with normal POSIX-style globs used generally by Garden. - // Note: We unfortunately can't exclude at this level because it simply doesn't work in git, for reasons unknown. - const patterns = [...(include || []).map((p) => ":(glob)" + p)] + + // Due to an issue in git, we can unfortunately only use _either_ include or exclude patterns in the + // ls-files commands, but not both. Trying both just ignores the exclude patterns. + + if (include?.includes("**/*")) { + // This is redundant + include = undefined + } + + const hasIncludes = !!include?.length + + const globalArgs = ["--glob-pathspecs"] const lsFilesCommonArgs = ["--cached", "--exclude", this.gardenDirPath] + if (!hasIncludes) { + for (const p of exclude) { + lsFilesCommonArgs.push("--exclude", p) + } + } + // List tracked but ignored files (we currently exclude those as well, so we need to query that specially) const trackedButIgnored = new Set( !this.ignoreFile ? [] - : await git("ls-files", "--ignored", ...lsFilesCommonArgs, "--exclude-per-directory", this.ignoreFile) + : await git( + ...globalArgs, + "ls-files", + "--ignored", + ...lsFilesCommonArgs, + "--exclude-per-directory", + this.ignoreFile + ) ) // List all submodule paths in the current path @@ -366,8 +389,12 @@ export class GitHandler extends VcsHandler { const resolvedPath = resolve(path, filePath) - // Filter on excludes - if (!matchPath(filePath, undefined, exclude) || submodulePaths.includes(resolvedPath)) { + // Filter on excludes and submodules + if (submodulePaths.includes(resolvedPath)) { + return + } + + if (hasIncludes && !matchPath(filePath, undefined, exclude)) { return } @@ -428,11 +455,11 @@ export class GitHandler extends VcsHandler { await new Promise((_resolve, _reject) => { // Prepare args - const args = ["ls-files", "-s", "--others", ...lsFilesCommonArgs] + const args = [...globalArgs, "ls-files", "-s", "--others", ...lsFilesCommonArgs] if (this.ignoreFile) { args.push("--exclude-per-directory", this.ignoreFile) } - args.push(...patterns) + args.push(...(include || [])) // Start git process gitLog.silly(`Calling git with args '${args.join(" ")}' in ${path}`) diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index 83c12b2444..8592a1de31 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -270,7 +270,7 @@ export const commonGitHandlerTests = (handlerCls: new (params: VcsHandlerParams) ).to.eql([]) }) - it("should respect include and exclude patterns, if both are specified", async () => { + it("should respect include and exclude filters, if both are specified", async () => { const moduleDir = resolve(tmpPath, "module-a") const pathA = resolve(moduleDir, "yes.txt") const pathB = resolve(tmpPath, "no.txt")