fix(git): fix exclude files handling in subtree Git repo scan mode (

#5504)

* refactor(test): own test helpers for different test contexts

* test(git): extra test case

Full (prefix and postfix) glob pattern for exclude filter

* test(git): split single context into 2 separate ones

* refactor: test helpers for deep nested dirs in exclude filter test

* test(git): reproducible failure example

* fix(git): handle directory exclusions properly

Normalize the excluded relative paths with redundant `..` and `.` parts.
This ensures that `--glob-pathspecs` Git flag will behave properly with `--exclude` arg.
This fix prevent rebuilding container module on excluded files modification.

* chore: extract some local vars for better readability and easier debugging

* test: enable fixed tests and mute the known failures

* refactor(git): move include/exclude files processing to a dedicated function

* improvement(git): always use normalized exclude paths in `subtree` scan mode

* improvement(git): always use normalized exclude paths in `repo` scan mode

* refactor: simplify re-init of exclude filter

* chore: more explanation on the implemented fix

* chore: re-arranged code

First, process inclusions, and then exclusions.
For more clarity and consistency in the comments.

* chore: cosmetic amendments in the comments

core/src/vcs/git-repo.ts

@@ -6,17 +6,51 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-import { GitHandler, augmentGlobs } from "./git.js"
-import type { GetFilesParams, VcsFile } from "./vcs.js"
+import { augmentGlobs, GitHandler } from "./git.js"
+import type { BaseIncludeExcludeFiles, GetFilesParams, IncludeExcludeFilesHandler, VcsFile } from "./vcs.js"
 import { isDirectory, matchPath } from "../util/fs.js"
 import fsExtra from "fs-extra"
-const { pathExists } = fsExtra
 import { pathToCacheContext } from "../cache.js"
 import { FileTree } from "./file-tree.js"
-import { sep } from "path"
+import { normalize, sep } from "path"
+
+const { pathExists } = fsExtra
 
 type ScanRepoParams = Pick<GetFilesParams, "log" | "path" | "pathDescription" | "failOnPrompt">
 
+interface GitRepoGetFilesParams extends GetFilesParams {
+  scanFromProjectRoot: boolean
+}
+
+interface GitRepoIncludeExcludeFiles extends BaseIncludeExcludeFiles {
+  augmentedIncludes: string[]
+  augmentedExcludes: string[]
+}
+
+const getIncludeExcludeFiles: IncludeExcludeFilesHandler<GitRepoGetFilesParams, GitRepoIncludeExcludeFiles> = async (
+  params
+) => {
+  const { include, path, scanFromProjectRoot } = params
+
+  // Make sure action config is not mutated.
+  let exclude = !params.exclude ? [] : [...params.exclude]
+
+  // Do the same normalization of the excluded paths like in `GitHandler`.
+  // This might be redundant because the non-normalized paths will be handled by `augmentGlobs` below.
+  // But this brings no harm and makes the implementation more clear.
+  exclude = exclude.map(normalize)
+
+  // We allow just passing a path like `foo` as include and exclude params
+  // Those need to be converted to globs, but we don't want to touch existing globs
+  const augmentedIncludes = include ? await augmentGlobs(path, include) : ["**/*"]
+  const augmentedExcludes = await augmentGlobs(path, exclude || [])
+  if (scanFromProjectRoot) {
+    augmentedExcludes.push("**/.garden/**/*")
+  }
+
+  return { include, exclude, augmentedIncludes, augmentedExcludes }
+}
+
 export class GitRepoHandler extends GitHandler {
   override name = "git-repo"
 
@@ -58,20 +92,16 @@ export class GitRepoHandler extends GitHandler {
 
     const moduleFiles = fileTree.getFilesAtPath(path)
 
-    // We allow just passing a path like `foo` as include and exclude params
-    // Those need to be converted to globs, but we don't want to touch existing globs
-    const include = params.include ? await augmentGlobs(path, params.include) : ["**/*"]
-    const exclude = await augmentGlobs(path, params.exclude || [])
-
-    if (scanRoot === this.garden?.projectRoot) {
-      exclude.push("**/.garden/**/*")
-    }
+    const scanFromProjectRoot = scanRoot === this.garden?.projectRoot
+    const { augmentedExcludes, augmentedIncludes } = await getIncludeExcludeFiles({ ...params, scanFromProjectRoot })
 
     log.debug(
-      `Found ${moduleFiles.length} files in module path, filtering by ${include.length} include and ${exclude.length} exclude globs`
+      `Found ${moduleFiles.length} files in module path, filtering by ${augmentedIncludes.length} include and ${augmentedExcludes.length} exclude globs`
+    )
+    log.silly(() => `Include globs: ${augmentedIncludes.join(", ")}`)
+    log.silly(() =>
+      augmentedExcludes.length > 0 ? `Exclude globs: ${augmentedExcludes.join(", ")}` : "No exclude globs"
     )
-    log.silly(() => `Include globs: ${include.join(", ")}`)
-    log.silly(() => (exclude.length > 0 ? `Exclude globs: ${exclude.join(", ")}` : "No exclude globs"))
 
     const filtered = moduleFiles.filter(({ path: p }) => {
       if (filter && !filter(p)) {
@@ -83,7 +113,7 @@ export class GitRepoHandler extends GitHandler {
       // but that caused issues with the glob matching on windows due to backslashes
       const relativePath = p.replace(`${path}${sep}`, "")
       log.silly(() => `Checking if ${relativePath} matches include/exclude globs`)
-      return matchPath(relativePath, include, exclude)
+      return matchPath(relativePath, augmentedIncludes, augmentedExcludes)
     })
 
     log.debug(`Found ${filtered.length} files in module path after glob matching`)

core/src/vcs/git.ts

@@ -7,11 +7,19 @@
  */
 
 import { performance } from "perf_hooks"
-import { isAbsolute, join, posix, relative, resolve } from "path"
+import { isAbsolute, join, normalize, posix, relative, resolve } from "path"
 import { isString } from "lodash-es"
 import fsExtra from "fs-extra"
 import { PassThrough } from "stream"
-import type { GetFilesParams, RemoteSourceParams, VcsFile, VcsHandlerParams, VcsInfo } from "./vcs.js"
+import type {
+  BaseIncludeExcludeFiles,
+  GetFilesParams,
+  IncludeExcludeFilesHandler,
+  RemoteSourceParams,
+  VcsFile,
+  VcsHandlerParams,
+  VcsInfo,
+} from "./vcs.js"
 import { VcsHandler } from "./vcs.js"
 import type { GardenError } from "../exceptions.js"
 import { ChildProcessError, ConfigurationError, isErrnoException, RuntimeError } from "../exceptions.js"
@@ -66,6 +74,39 @@ export interface GitCli {
   (...args: (string | undefined)[]): Promise<string[]>
 }
 
+interface GitSubTreeIncludeExcludeFiles extends BaseIncludeExcludeFiles {
+  hasIncludes: boolean
+}
+
+const getIncludeExcludeFiles: IncludeExcludeFilesHandler<GetFilesParams, GitSubTreeIncludeExcludeFiles> = async (
+  params: GetFilesParams
+) => {
+  let include = params.include
+
+  // We 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.
+
+  // 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
+
+  // Make sure action config is not mutated.
+  let exclude = !params.exclude ? [] : [...params.exclude]
+
+  // It looks like relative paths with redundant '.' and '..' parts
+  // do not work well along with `--exclude` and `--glob-pathspecs` flags.
+  // So, we need to normalize paths like './dir' to be just 'dir',
+  // otherwise such dirs won't be excluded by `--exclude` flag applied with `--glob-pathspecs`.
+  exclude = [...exclude.map(normalize), "**/.garden/**/*"]
+
+  return { include, exclude, hasIncludes }
+}
+
 interface Submodule {
   path: string
   url: string
@@ -153,19 +194,13 @@ export class GitHandler extends VcsHandler {
    * so that {@link getFiles} won't refer to the method in the subclass.
    */
   async _getFiles(params: GetFilesParams): Promise<VcsFile[]> {
-    const { log, path, pathDescription = "directory", filter, failOnPrompt = false } = params
-    let { include, exclude } = params
-
-    if (include && include.length === 0) {
+    if (params.include && params.include.length === 0) {
       // No need to proceed, nothing should be included
       return []
     }
 
-    if (!exclude) {
-      exclude = []
-    }
-    // Make sure action config is not mutated
-    exclude = [...exclude, "**/.garden/**/*"]
+    const { log, path, pathDescription = "directory", filter, failOnPrompt = false } = params
+    const { exclude, hasIncludes, include } = await getIncludeExcludeFiles(params)
 
     const gitLog = log
       .createLog({ name: "git" })
@@ -203,21 +238,6 @@ export class GitHandler extends VcsHandler {
         .map((modifiedRelPath) => resolve(gitRoot, modifiedRelPath))
     )
 
-    const absExcludes = exclude.map((p) => resolve(path, p))
-
-    // 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.
-
-    // 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]
 
@@ -256,11 +276,12 @@ export class GitHandler extends VcsHandler {
       // Need to automatically add `**/*` to directory paths, to match git behavior when filtering.
       const augmentedIncludes = await augmentGlobs(path, include)
       const augmentedExcludes = await augmentGlobs(path, exclude)
+      const absExcludes = exclude.map((p) => resolve(path, p))
 
       // Resolve submodules
       // TODO: see about optimizing this, avoiding scans when we're sure they'll not match includes/excludes etc.
       submoduleFiles = submodulePaths.map(async (submodulePath) => {
-        if (!submodulePath.startsWith(path) || absExcludes?.includes(submodulePath)) {
+        if (!submodulePath.startsWith(path) || absExcludes.includes(submodulePath)) {
           return []
         }
 
@@ -346,7 +367,8 @@ export class GitHandler extends VcsHandler {
         return
       }
 
-      if (hasIncludes && !matchPath(filePath, undefined, exclude)) {
+      const passesExclusionFilter = matchPath(filePath, undefined, exclude)
+      if (hasIncludes && !passesExclusionFilter) {
         return
       }
 
@@ -714,8 +736,9 @@ export async function augmentGlobs(basePath: string, globs?: string[]): Promise<
       }
 
       try {
-        const isDir = (await stat(joinWithPosix(basePath, pattern))).isDirectory()
-        return isDir ? posix.join(pattern, "**", "*") : pattern
+        const path = joinWithPosix(basePath, pattern)
+        const stats = await stat(path)
+        return stats.isDirectory() ? posix.join(pattern, "**", "*") : pattern
       } catch {
         return pattern
       }

core/src/vcs/vcs.ts

@@ -34,6 +34,7 @@ import type { Garden } from "../garden.js"
 import { Profile } from "../util/profiling.js"
 
 import AsyncLock from "async-lock"
+
 const scanLock = new AsyncLock()
 
 export const versionStringPrefix = "v-"
@@ -121,6 +122,15 @@ export interface GetFilesParams {
   scanRoot: string | undefined
 }
 
+export interface BaseIncludeExcludeFiles {
+  include?: string[]
+  exclude: string[]
+}
+
+export type IncludeExcludeFilesHandler<T extends GetFilesParams, R extends BaseIncludeExcludeFiles> = (
+  params: T
+) => Promise<R>
+
 export interface GetTreeVersionParams {
   log: Log
   projectName: string

core/test/data/test-projects/include-exclude/module-b/dir1/foo.txt

@@ -0,0 +1 @@
+I'm in the excluded dir. I should be excluded too.

core/test/data/test-projects/include-exclude/module-b/dir2/bar.txt

@@ -0,0 +1 @@
+I'm in the excluded dir. I should be excluded.

core/test/data/test-projects/include-exclude/module-b/garden.yml

@@ -1,4 +1,5 @@
 kind: Module
 name: module-b
 type: test
-exclude: ["nope.*"]
+# test both dir name and relative path
+exclude: ["nope.*", "./dir1", "dir2"]