Skip to content

Commit

Permalink
perf(git): avoid unnecessary file hashing while config files detection (
Browse files Browse the repository at this point in the history
#6461)

* perf(git): skip unnecessary hash calculation on config scan

* refactor(git): replace positional args with parameter object

* fix(git): optionally skip hash calculation only for untracked files

* refactor: rename control flag and inverse its semantics

* refactor: flatten conditional logic

* chore(test): remove unnecessary local state
  • Loading branch information
vvagaytsev authored Sep 25, 2024
1 parent 3aa0b23 commit a786a50
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 24 deletions.
1 change: 1 addition & 0 deletions core/src/util/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export async function findConfigPathsInPath({
log,
filter: (f) => isConfigFilename(basename(f)),
scanRoot: dir,
hashUntrackedFiles: false,
})

return paths.map((f) => f.path)
Expand Down
3 changes: 2 additions & 1 deletion core/src/vcs/git-repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { realpath } from "fs/promises"

const { pathExists } = fsExtra

type ScanRepoParams = Pick<GetFilesParams, "log" | "path" | "pathDescription" | "failOnPrompt">
type ScanRepoParams = Pick<GetFilesParams, "log" | "path" | "pathDescription" | "failOnPrompt" | "hashUntrackedFiles">

interface GitRepoGetFilesParams extends GetFilesParams {
scanFromProjectRoot: boolean
Expand Down Expand Up @@ -162,6 +162,7 @@ export class GitRepoHandler extends AbstractGitHandler {
path: scanRoot,
pathDescription: pathDescription || "repository",
failOnPrompt,
hashUntrackedFiles: params.hashUntrackedFiles,
})

const filesAtPath = fileTree.getFilesAtPath(path)
Expand Down
78 changes: 60 additions & 18 deletions core/src/vcs/git-sub-tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export class GitSubTreeHandler extends AbstractGitHandler {
return []
}

const { log, path, pathDescription = "directory", filter, failOnPrompt = false } = params
const { log, path, pathDescription = "directory", filter, failOnPrompt = false, hashUntrackedFiles = true } = params

const gitLog = log
.createLog({ name: "git" })
Expand Down Expand Up @@ -246,6 +246,7 @@ export class GitSubTreeHandler extends AbstractGitHandler {
matchPath(join(submoduleRelPath, p), augmentedIncludes, augmentedExcludes) && (!filter || filter(p)),
scanRoot: submodulePath,
failOnPrompt,
hashUntrackedFiles,
})
})
}
Expand Down Expand Up @@ -290,7 +291,12 @@ export class GitSubTreeHandler extends AbstractGitHandler {
// No need to stat unless it has no hash, is a symlink, or is modified
// Note: git ls-files always returns mode 120000 for symlinks
if (hash && entry.mode !== "120000" && !modifiedFiles.has(resolvedPath)) {
return ensureHash(output, undefined, modifiedFiles)
return ensureHash({
file: output,
stats: undefined,
modifiedFiles,
hashUntrackedFiles,
})
}

try {
Expand All @@ -313,7 +319,12 @@ export class GitSubTreeHandler extends AbstractGitHandler {
gitLog.verbose(`Ignoring symlink pointing outside of ${pathDescription} at ${resolvedPath}`)
return
}
return ensureHash(output, stats, modifiedFiles)
return ensureHash({
file: output,
stats,
modifiedFiles,
hashUntrackedFiles,
})
} catch (err) {
if (isErrnoException(err) && err.code === "ENOENT") {
gitLog.verbose(`Ignoring dead symlink at ${resolvedPath}`)
Expand All @@ -322,10 +333,20 @@ export class GitSubTreeHandler extends AbstractGitHandler {
throw err
}
} else {
return ensureHash(output, stats, modifiedFiles)
return ensureHash({
file: output,
stats,
modifiedFiles,
hashUntrackedFiles,
})
}
} else {
return ensureHash(output, stats, modifiedFiles)
return ensureHash({
file: output,
stats,
modifiedFiles,
hashUntrackedFiles,
})
}
} catch (err) {
if (isErrnoException(err) && err.code === "ENOENT") {
Expand Down Expand Up @@ -465,21 +486,42 @@ function parseGitLsFilesOutputLine(data: Buffer): GitEntry | undefined {
/**
* Make sure we have a fresh hash for each file.
*/
async function ensureHash(
file: VcsFile,
stats: fsExtra.Stats | undefined,
async function ensureHash({
file,
stats,
modifiedFiles,
hashUntrackedFiles,
}: {
file: VcsFile
stats: fsExtra.Stats | undefined
modifiedFiles: Set<string>
): Promise<VcsFile> {
if (file.hash === "" || modifiedFiles.has(file.path)) {
// Don't attempt to hash directories. Directories (which will only come up via symlinks btw)
// will by extension be filtered out of the list.
if (stats && !stats.isDirectory()) {
const hash = await hashObject(stats, file.path)
if (hash !== "") {
file.hash = hash
return file
}
hashUntrackedFiles: boolean
}): Promise<VcsFile> {
// If the file has not been modified, then it's either committed or untracked.
if (!modifiedFiles.has(file.path)) {
// If the hash is already defined, then the file is committed and its hash is up-to-date.
if (file.hash !== "") {
return file
}

// Otherwise, the file is untracked.
if (!hashUntrackedFiles) {
// So we can skip its hash calculation if we don't need the hashes of untracked files.
// Hashes can be skipped while scanning the FS for Garden config files.
return file
}
}

// Don't attempt to hash directories. Directories (which will only come up via symlinks btw)
// will by extension be filtered out of the list.
if (!stats || stats.isDirectory()) {
return file
}

const hash = await hashObject(stats, file.path)
if (hash !== "") {
file.hash = hash
}

return file
}
1 change: 1 addition & 0 deletions core/src/vcs/vcs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export interface GetFilesParams {
filter?: (path: string) => boolean
failOnPrompt?: boolean
scanRoot: string | undefined
hashUntrackedFiles?: boolean
}

export interface BaseIncludeExcludeFiles {
Expand Down
7 changes: 2 additions & 5 deletions core/test/unit/src/commands/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { taskResultOutputs, getAllTaskResults } from "../../../helpers.js"
import type { ModuleConfig } from "../../../../src/config/module.js"
import type { Log } from "../../../../src/logger/log-entry.js"
import fsExtra from "fs-extra"

const { writeFile } = fsExtra
import { join } from "path"
import type { ProcessCommandResult } from "../../../../src/commands/base.js"
Expand Down Expand Up @@ -324,10 +325,8 @@ describe("BuildCommand", () => {
})

it("should rebuild module if a deep dependency has been modified", async () => {
let garden = await getFreshTestGarden()

const { result: result1 } = await buildCommand.action({
garden,
garden: await getFreshTestGarden(),
...defaultOpts,
args: { names: ["aaa-service"] },
opts: withDefaultGlobalOpts({ "watch": false, "force": true, "with-dependants": false }),
Expand All @@ -337,8 +336,6 @@ describe("BuildCommand", () => {

await writeFile(join(projectPath, "C/file.txt"), "module c has been modified")

garden = await getFreshTestGarden()

const { result: result2 } = await buildCommand.action({
garden: await getFreshTestGarden(),
...defaultOpts,
Expand Down

0 comments on commit a786a50

Please sign in to comment.