fix(vcs): use structural path comparison to compute minimal repo roots (

#5867)

* fix(git): type-safe caching for repo roots

Added undefined- and null-safe checkers.
`Map.has()` can return `true` if any key has an explicit `null` or `undefined` value.

* fix(vcs): use structural comparison to check sub-paths

Compare each path level-by-level instead of using substring-bases checks.

* chore(vcs): more detailed error messages

* chore: remove unused helper

* chore(git): log project scan info on verbose log-level

* chore(git): print project scan duration in logs

* chore: named constant for cache dir name

* chore: add side TODO-comment

It will be addressed in a separate PR if necessary.

* chore: named interface for project scan config

* chore: simplified syntax

* refactor(git): extract `getCli` as a standalone function

* refactor(git): move helper `getModifiedFiles` outside `GitHandler` class

* refactor(git): rename method `getRemoteSourceLock` -> `withRemoteSourceLock`

* chore: linting

core/src/config/project.ts

@@ -195,6 +195,12 @@ interface GitConfig {
   mode: GitScanMode
 }
 
+interface ProjectScan {
+  include?: string[]
+  exclude?: string[]
+  git?: GitConfig
+}
+
 export interface ProjectConfig extends BaseGardenResource {
   apiVersion: GardenApiVersion
   kind: "Project"
@@ -208,11 +214,7 @@ export interface ProjectConfig extends BaseGardenResource {
   dotIgnoreFile: string
   dotIgnoreFiles?: string[]
   environments: EnvironmentConfig[]
-  scan?: {
-    include?: string[]
-    exclude?: string[]
-    git?: GitConfig
-  }
+  scan?: ProjectScan
   outputs?: OutputSpec[]
   providers: GenericProviderConfig[]
   sources?: SourceConfig[]

core/src/constants.ts

@@ -26,6 +26,7 @@ export const GARDEN_CORE_ROOT = !!extractedRoot
 export const GARDEN_CLI_ROOT = resolve(GARDEN_CORE_ROOT, "..", "cli")
 export const STATIC_DIR = !!extractedRoot ? resolve(extractedRoot, "static") : resolve(GARDEN_CORE_ROOT, "..", "static")
 export const DEFAULT_GARDEN_DIR_NAME = ".garden"
+export const CACHE_DIR_NAME = "cache"
 export const MUTAGEN_DIR_NAME = "mutagen"
 export const LOGS_DIR_NAME = "logs"
 export const GARDEN_GLOBAL_PATH = join(homedir(), DEFAULT_GARDEN_DIR_NAME)

core/src/plugins/container/helpers.ts

@@ -28,7 +28,6 @@ import titleize from "titleize"
 import { deline, stripQuotes, splitLast, splitFirst } from "../../util/string.js"
 import type { PluginContext } from "../../plugin-context.js"
 import type { ModuleVersion } from "../../vcs/vcs.js"
-import type { SpawnParams } from "../../util/ext-tools.js"
 import type { ContainerBuildAction } from "./config.js"
 import { defaultDockerfileName } from "./config.js"
 import { joinWithPosix } from "../../util/fs.js"
@@ -385,11 +384,6 @@ const helpers = {
     }
   },
 
-  spawnDockerCli(params: SpawnParams & { ctx: PluginContext }) {
-    const docker = params.ctx.tools["container.docker"]
-    return docker.spawn(params)
-  },
-
   moduleHasDockerfile(config: ContainerModuleConfig, version: ModuleVersion): boolean {
     // If we explicitly set a Dockerfile, we take that to mean you want it to be built.
     // If the file turns out to be missing, this will come up in the build handler.

core/src/tasks/resolve-provider.ts

@@ -23,18 +23,19 @@ import { defaultEnvironmentStatus } from "../plugin/handlers/Provider/getEnviron
 import { getPluginBases, getPluginBaseNames } from "../plugins.js"
 import { Profile } from "../util/profiling.js"
 import { join, dirname } from "path"
-import fsExtra from "fs-extra"
-const { readFile, writeFile, ensureDir } = fsExtra
 import { deserialize, serialize } from "v8"
 import { environmentStatusSchema } from "../config/status.js"
 import { hashString, isNotNull } from "../util/util.js"
-import { gardenEnv } from "../constants.js"
+import { CACHE_DIR_NAME, gardenEnv } from "../constants.js"
 import { stableStringify } from "../util/string.js"
 import { OtelTraced } from "../util/open-telemetry/decorators.js"
 import { LogLevel } from "../logger/logger.js"
 import type { Log } from "../logger/log-entry.js"
 import { styles } from "../logger/styles.js"
 import type { ObjectPath } from "../config/base.js"
+import fsExtra from "fs-extra"
+
+const { readFile, writeFile, ensureDir } = fsExtra
 
 /**
  * Returns a provider log context with the provider name set.
@@ -454,5 +455,5 @@ export function getProviderStatusCachePath({
   gardenDirPath: string
   pluginName: string
 }) {
-  return join(gardenDirPath, "cache", "provider-statuses", `${pluginName}.json`)
+  return join(gardenDirPath, CACHE_DIR_NAME, "provider-statuses", `${pluginName}.json`)
 }

core/src/util/testing.ts

@@ -219,8 +219,8 @@ export class TestGarden extends Garden {
     } else {
       params = await resolveGardenParams(currentDirectory, { commandInfo: defaultCommandInfo, ...opts })
       if (opts?.gitScanMode) {
-        params.projectConfig.scan = params.projectConfig.scan ?? { git: { mode: opts.gitScanMode } }
-        params.projectConfig.scan.git = params.projectConfig.scan.git ?? { mode: opts.gitScanMode }
+        params.projectConfig.scan ??= { git: { mode: opts.gitScanMode } }
+        params.projectConfig.scan.git ??= { mode: opts.gitScanMode }
         params.projectConfig.scan.git.mode = opts.gitScanMode
       }
       if (cacheKey) {

core/src/vcs/git.ts

@@ -27,6 +27,7 @@ import { getStatsType, joinWithPosix, matchPath } from "../util/fs.js"
 import { dedent, deline, splitLast } from "../util/string.js"
 import { defer, exec } from "../util/util.js"
 import type { Log } from "../logger/log-entry.js"
+import { renderDuration } from "../logger/util.js"
 import parseGitConfig from "parse-git-config"
 import type { Profiler } from "../util/profiling.js"
 import { getDefaultProfiler, Profile } from "../util/profiling.js"
@@ -74,6 +75,34 @@ export interface GitCli {
   (...args: (string | undefined)[]): Promise<string[]>
 }
 
+export function gitCli(log: Log, cwd: string, failOnPrompt = false): GitCli {
+  /**
+   * @throws ChildProcessError
+   */
+  return async (...args: (string | undefined)[]) => {
+    log.silly(`Calling git with args '${args.join(" ")}' in ${cwd}`)
+    const { stdout } = await exec("git", args.filter(isString), {
+      cwd,
+      maxBuffer: 10 * 1024 * 1024,
+      env: failOnPrompt ? { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "true" } : undefined,
+    })
+    return stdout.split("\n").filter((line) => line.length > 0)
+  }
+}
+
+async function getModifiedFiles(git: GitCli, path: string) {
+  try {
+    return await git("diff-index", "--name-only", "HEAD", path)
+  } catch (err) {
+    if (err instanceof ChildProcessError && err.details.code === 128) {
+      // no commit in repo
+      return []
+    } else {
+      throw err
+    }
+  }
+}
+
 interface GitSubTreeIncludeExcludeFiles extends BaseIncludeExcludeFiles {
   hasIncludes: boolean
 }
@@ -116,7 +145,7 @@ interface Submodule {
 @Profile()
 export class GitHandler extends VcsHandler {
   name = "git"
-  repoRoots = new Map()
+  repoRoots = new Map<string, string>()
   profiler: Profiler
   protected lock: AsyncLock
 
@@ -126,48 +155,21 @@ export class GitHandler extends VcsHandler {
     this.lock = new AsyncLock()
   }
 
-  gitCli(log: Log, cwd: string, failOnPrompt = false): GitCli {
-    /**
-     * @throws ChildProcessError
-     */
-    return async (...args: (string | undefined)[]) => {
-      log.silly(`Calling git with args '${args.join(" ")}' in ${cwd}`)
-      const { stdout } = await exec("git", args.filter(isString), {
-        cwd,
-        maxBuffer: 10 * 1024 * 1024,
-        env: failOnPrompt ? { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "true" } : undefined,
-      })
-      return stdout.split("\n").filter((line) => line.length > 0)
-    }
-  }
-
-  private async getModifiedFiles(git: GitCli, path: string) {
-    try {
-      return await git("diff-index", "--name-only", "HEAD", path)
-    } catch (err) {
-      if (err instanceof ChildProcessError && err.details.code === 128) {
-        // no commit in repo
-        return []
-      } else {
-        throw err
-      }
-    }
-  }
-
-  async getRepoRoot(log: Log, path: string, failOnPrompt = false) {
-    if (this.repoRoots.has(path)) {
-      return this.repoRoots.get(path)
+  async getRepoRoot(log: Log, path: string, failOnPrompt = false): Promise<string> {
+    const repoRoot = this.repoRoots.get(path)
+    if (!!repoRoot) {
+      return repoRoot
     }
 
     // Make sure we're not asking concurrently for the same root
     return this.lock.acquire(`repo-root:${path}`, async () => {
-      if (this.repoRoots.has(path)) {
-        return this.repoRoots.get(path)
+      const repoRoot = this.repoRoots.get(path)
+      if (!!repoRoot) {
+        return repoRoot
       }
 
-      const git = this.gitCli(log, path, failOnPrompt)
-
       try {
+        const git = gitCli(log, path, failOnPrompt)
         const repoRoot = (await git("rev-parse", "--show-toplevel"))[0]
         this.repoRoots.set(path, repoRoot)
         return repoRoot
@@ -203,7 +205,7 @@ export class GitHandler extends VcsHandler {
 
     const gitLog = log
       .createLog({ name: "git" })
-      .debug(
+      .verbose(
         `Scanning ${pathDescription} at ${path}\n  → Includes: ${params.include || "(none)"}\n  → Excludes: ${
           params.exclude || "(none)"
         }`
@@ -228,12 +230,12 @@ export class GitHandler extends VcsHandler {
 
     let files: VcsFile[] = []
 
-    const git = this.gitCli(gitLog, path, failOnPrompt)
+    const git = gitCli(gitLog, path, failOnPrompt)
     const gitRoot = await this.getRepoRoot(gitLog, path, failOnPrompt)
 
     // List modified files, so that we can ensure we have the right hash for them later
     const modified = new Set(
-      (await this.getModifiedFiles(git, path))
+      (await getModifiedFiles(git, path))
         // The output here is relative to the git root, and not the directory `path`
         .map((modifiedRelPath) => resolve(gitRoot, modifiedRelPath))
     )
@@ -476,7 +478,7 @@ export class GitHandler extends VcsHandler {
     await processEnded.promise
     await queue.onIdle()
 
-    gitLog.debug(`Found ${count} files in ${pathDescription} ${path}`)
+    gitLog.verbose(`Found ${count} files in ${pathDescription} ${path} ${renderDuration(gitLog.getDuration())}`)
 
     // We have done the processing of this level of files
     // So now we just have to wait for all the recursive submodules to resolve as well
@@ -496,7 +498,7 @@ export class GitHandler extends VcsHandler {
     failOnPrompt = false
   ) {
     await ensureDir(absPath)
-    const git = this.gitCli(log, absPath, failOnPrompt)
+    const git = gitCli(log, absPath, failOnPrompt)
     // Use `--recursive` to include submodules
     if (!isSha1(hash)) {
       return git(
@@ -532,7 +534,7 @@ export class GitHandler extends VcsHandler {
 
   // TODO Better auth handling
   async ensureRemoteSource({ url, name, log, sourceType, failOnPrompt = false }: RemoteSourceParams): Promise<string> {
-    return this.getRemoteSourceLock(sourceType, name, async () => {
+    return this.withRemoteSourceLock(sourceType, name, async () => {
       const remoteSourcesPath = this.getRemoteSourcesLocalPath(sourceType)
       await ensureDir(remoteSourcesPath)
 
@@ -561,12 +563,12 @@ export class GitHandler extends VcsHandler {
 
   async updateRemoteSource({ url, name, sourceType, log, failOnPrompt = false }: RemoteSourceParams) {
     const absPath = this.getRemoteSourceLocalPath(name, url, sourceType)
-    const git = this.gitCli(log, absPath, failOnPrompt)
+    const git = gitCli(log, absPath, failOnPrompt)
     const { repositoryUrl, hash } = parseGitUrl(url)
 
     await this.ensureRemoteSource({ url, name, sourceType, log, failOnPrompt })
 
-    await this.getRemoteSourceLock(sourceType, name, async () => {
+    await this.withRemoteSourceLock(sourceType, name, async () => {
       const gitLog = log.createLog({ name, showDuration: true }).info("Getting remote state")
       await git("remote", "update")
 
@@ -595,7 +597,7 @@ export class GitHandler extends VcsHandler {
     })
   }
 
-  private getRemoteSourceLock(sourceType: string, name: string, func: () => Promise<any>) {
+  private withRemoteSourceLock(sourceType: string, name: string, func: () => Promise<any>) {
     return this.lock.acquire(`remote-source-${sourceType}-${name}`, func)
   }
 
@@ -668,7 +670,7 @@ export class GitHandler extends VcsHandler {
   }
 
   async getPathInfo(log: Log, path: string, failOnPrompt = false): Promise<VcsInfo> {
-    const git = this.gitCli(log, path, failOnPrompt)
+    const git = gitCli(log, path, failOnPrompt)
 
     const output: VcsInfo = {
       branch: "",

core/src/vcs/vcs.ts

@@ -8,7 +8,7 @@
 
 import { sortBy, pick } from "lodash-es"
 import { createHash } from "node:crypto"
-import { relative, sep } from "path"
+import { isAbsolute, relative, sep } from "path"
 import fsExtra from "fs-extra"
 import { dirname } from "node:path"
 
@@ -35,6 +35,7 @@ import { Profile } from "../util/profiling.js"
 
 import AsyncLock from "async-lock"
 import { makeDocsLinkStyled } from "../docs/common.js"
+import { RuntimeError } from "../exceptions.js"
 
 const scanLock = new AsyncLock()
 
@@ -332,7 +333,7 @@ export abstract class VcsHandler {
         if (!outputs[path]) {
           // No path set so far
           outputs[path] = repoPath
-        } else if (outputs[path].startsWith(repoPath)) {
+        } else if (isSubPath(repoPath, outputs[path])) {
           // New path is prefix of prior path
           outputs[path] = repoPath
         } else {
@@ -345,7 +346,7 @@ export abstract class VcsHandler {
               // Don't go past the actual git repo root
               outputs[path] = repoRoot
               break
-            } else if (outputs[path].startsWith(p)) {
+            } else if (isSubPath(p, outputs[path])) {
               // Found a common prefix
               outputs[path] = p
               break
@@ -462,3 +463,36 @@ export function getSourcePath(config: ModuleConfig | BaseActionConfig) {
 export function describeConfig(config: ModuleConfig | BaseActionConfig): ActionDescription | ModuleDescription {
   return isActionConfig(config) ? `${config.kind} action ${config.name}` : `module ${config.name}`
 }
+
+/**
+ * Checks if the {@code subPathCandidate} is a sub-path of {@code basePath}.
+ * Sub-path means that a candidate must be located inside a reference path.
+ *
+ * Both {@basePath} and {@ subPathCandidate} must be absolute paths
+ *
+ * @param basePath the reference path (absolute)
+ * @param subPathCandidate the path to be checked (absolute)
+ */
+export function isSubPath(basePath: string, subPathCandidate: string): boolean {
+  if (!isAbsolute(basePath)) {
+    throw new RuntimeError({ message: `Expected absolute path as a base path, but got: ${basePath}` })
+  }
+  if (!isAbsolute(subPathCandidate)) {
+    throw new RuntimeError({ message: `Expected absolute path as a sub-path candidate, but got ${subPathCandidate}` })
+  }
+
+  const pathParts = basePath.split(sep)
+  const subPathCandidateParts = subPathCandidate.split(sep)
+
+  if (subPathCandidateParts.length < pathParts.length) {
+    return false
+  }
+
+  for (let i = 0; i < pathParts.length; i++) {
+    if (pathParts[i] !== subPathCandidateParts[i]) {
+      return false
+    }
+  }
+
+  return true
+}

core/test/unit/src/tasks/resolve-provider.ts

@@ -15,11 +15,13 @@ import type { TempDirectory, TestGarden } from "../../../helpers.js"
 import { makeTempDir, makeTestGarden, stubProviderAction, createProjectConfig } from "../../../helpers.js"
 import { ResolveProviderTask } from "../../../../src/tasks/resolve-provider.js"
 import fsExtra from "fs-extra"
+
 const { pathExists, writeFile, remove } = fsExtra
 import { join } from "path"
 import { serialize } from "v8"
 import moment from "moment"
 import { GraphResults } from "../../../../src/graph/results.js"
+import { CACHE_DIR_NAME } from "../../../../src/constants.js"
 
 describe("ResolveProviderTask", () => {
   let tmpDir: TempDirectory
@@ -35,7 +37,8 @@ describe("ResolveProviderTask", () => {
   })
 
   beforeEach(async () => {
-    await remove(join(tmpDir.path, "cache"))
+    // TODO: it looks like this path never exists => fix it?
+    await remove(join(tmpDir.path, CACHE_DIR_NAME))
 
     garden = await makeTestGarden(tmpDir.path, {
       config: createProjectConfig({