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

perf(git): avoid unnecessary file hashing while config files detection #6461

Merged
merged 6 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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