Fix behaviour when discovering files for global hashing (#6738)

### Description

When turbo tries to discover files for the global hash we were
incorrectly yielding folders which were being passed on to git. This
adds a unit and integration test to ensure this doesn't happen, and
fixes the bug.

Closes #6729 and supersedes #6736

### Testing Instructions

Run unit tests or integration tests


Closes TURBO-1875

---------

Co-authored-by: Greg Soltis <Greg Soltis>

crates/turborepo-lib/src/run/global_hash.rs

@@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet};
 use globwalk::WalkType;
 use thiserror::Error;
 use tracing::debug;
-use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf};
+use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, RelativeUnixPathBuf};
 use turborepo_env::{get_global_hashable_env_vars, DetailedMap, EnvironmentVariableMap};
 use turborepo_lockfiles::Lockfile;
 use turborepo_repository::package_manager::{self, PackageManager};
@@ -68,47 +68,8 @@ pub fn get_global_hash_inputs<'a, L: ?Sized + Lockfile>(
         global_hashable_env_vars.all.names()
     );
 
-    let mut global_deps = HashSet::new();
-
-    if !global_file_dependencies.is_empty() {
-        let exclusions = match package_manager.get_workspace_globs(root_path) {
-            Ok(globs) => globs.raw_exclusions,
-            // If we hit a missing workspaces error, we could be in single package mode
-            // so we should just use the default globs
-            Err(package_manager::Error::Workspace(_)) => {
-                package_manager.get_default_exclusions().collect()
-            }
-            Err(err) => {
-                debug!("no workspace globs found");
-                return Err(err.into());
-            }
-        };
-
-        // This is a bit of a hack to ensure that we don't crash
-        // when given an absolute path on Windows. We don't support
-        // absolute paths, but the ':' from the drive letter will also
-        // fail to compile to a glob. We already know we aren't going to
-        // get anything good, and we've already logged a warning, but we
-        // can modify the glob so it compiles. This is similar to the old
-        // behavior, which tacked it on to the end of the base path unmodified,
-        // and then would produce no files.
-        #[cfg(windows)]
-        let windows_global_file_dependencies: Vec<String> = global_file_dependencies
-            .iter()
-            .map(|s| s.replace(":", ""))
-            .collect();
-        let files = globwalk::globwalk(
-            root_path,
-            #[cfg(not(windows))]
-            global_file_dependencies,
-            #[cfg(windows)]
-            windows_global_file_dependencies.as_slice(),
-            &exclusions,
-            WalkType::All,
-        )?;
-
-        global_deps.extend(files);
-    }
+    let mut global_deps =
+        collect_global_deps(package_manager, root_path, global_file_dependencies)?;
 
     if lockfile.is_none() {
         global_deps.insert(root_path.join_component("package.json"));
@@ -158,6 +119,52 @@ pub fn get_global_hash_inputs<'a, L: ?Sized + Lockfile>(
     })
 }
 
+fn collect_global_deps(
+    package_manager: &PackageManager,
+    root_path: &AbsoluteSystemPath,
+    global_file_dependencies: &[String],
+) -> Result<HashSet<AbsoluteSystemPathBuf>, Error> {
+    if global_file_dependencies.is_empty() {
+        return Ok(HashSet::new());
+    }
+    let exclusions = match package_manager.get_workspace_globs(root_path) {
+        Ok(globs) => globs.raw_exclusions,
+        // If we hit a missing workspaces error, we could be in single package mode
+        // so we should just use the default globs
+        Err(package_manager::Error::Workspace(_)) => {
+            package_manager.get_default_exclusions().collect()
+        }
+        Err(err) => {
+            debug!("no workspace globs found");
+            return Err(err.into());
+        }
+    };
+
+    // This is a bit of a hack to ensure that we don't crash
+    // when given an absolute path on Windows. We don't support
+    // absolute paths, but the ':' from the drive letter will also
+    // fail to compile to a glob. We already know we aren't going to
+    // get anything good, and we've already logged a warning, but we
+    // can modify the glob so it compiles. This is similar to the old
+    // behavior, which tacked it on to the end of the base path unmodified,
+    // and then would produce no files.
+    #[cfg(windows)]
+    let windows_global_file_dependencies: Vec<String> = global_file_dependencies
+        .iter()
+        .map(|s| s.replace(":", ""))
+        .collect();
+
+    Ok(globwalk::globwalk(
+        root_path,
+        #[cfg(not(windows))]
+        global_file_dependencies,
+        #[cfg(windows)]
+        windows_global_file_dependencies.as_slice(),
+        &exclusions,
+        WalkType::Files,
+    )?)
+}
+
 impl<'a> GlobalHashableInputs<'a> {
     pub fn calculate_global_hash_from_inputs(&mut self) -> String {
         match self.env_mode {
@@ -206,7 +213,7 @@ mod tests {
     use turborepo_repository::package_manager::PackageManager;
 
     use super::get_global_hash_inputs;
-    use crate::cli::EnvMode;
+    use crate::{cli::EnvMode, run::global_hash::collect_global_deps};
 
     #[test]
     fn test_absolute_path() {
@@ -244,4 +251,43 @@ mod tests {
         );
         assert!(result.is_ok());
     }
+
+    /// get_global_hash_inputs should not yield any folders when walking since
+    /// turbo does not consider changes to folders when evaluating hashes,
+    /// only to files
+    #[test]
+    fn test_collect_only_yields_files() {
+        let tmp = tempfile::tempdir().unwrap();
+
+        // add some files
+        //   - package.json
+        //   - src/index.js
+        //   - src/index.test.js
+        //   - empty-folder/
+
+        let root = AbsoluteSystemPathBuf::try_from(tmp.path()).unwrap();
+        let src = root.join_component("src");
+
+        root.join_component("package.json")
+            .create_with_contents("{}")
+            .unwrap();
+        root.join_component("empty-folder")
+            .create_dir_all()
+            .unwrap();
+
+        src.create_dir_all().unwrap();
+        src.join_component("index.js")
+            .create_with_contents("console.log('hello world');")
+            .unwrap();
+        src.join_component("index.test.js")
+            .create_with_contents("")
+            .unwrap();
+
+        let global_file_dependencies = vec!["**".to_string()];
+        let results =
+            collect_global_deps(&PackageManager::Berry, &root, &global_file_dependencies).unwrap();
+
+        // should not yield the root folder itself, src, or empty-folder
+        assert_eq!(results.len(), 3, "{:?}", results);
+    }
 }

turborepo-tests/integration/fixtures/global_deps/.gitignore

@@ -0,0 +1,3 @@
+node_modules/
+.turbo
+.npmrc

turborepo-tests/integration/fixtures/global_deps/apps/my-app/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "my-app",
+  "scripts": {
+    "build": "echo building",
+    "maybefails": "exit 4"
+  },
+  "dependencies": {
+    "util": "*"
+  }
+}

turborepo-tests/integration/fixtures/global_deps/global_deps/bar.txt

@@ -0,0 +1 @@
+other file, not a global dependency

turborepo-tests/integration/fixtures/global_deps/global_deps/foo.txt

@@ -0,0 +1 @@
+global dep! all tasks depend on this content!

turborepo-tests/integration/fixtures/global_deps/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "monorepo",
+  "scripts": {
+    "something": "turbo run build"
+  },
+  "workspaces": [
+    "apps/**",
+    "packages/**"
+  ]
+}

turborepo-tests/integration/fixtures/global_deps/packages/another/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "another",
+  "scripts": {}
+}

turborepo-tests/integration/fixtures/global_deps/packages/util/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "util",
+  "scripts": {
+    "build": "echo building",
+    "maybefails": "echo didnotfail"
+  }
+}

turborepo-tests/integration/fixtures/global_deps/turbo.json

@@ -0,0 +1,21 @@
+{
+  "$schema": "https://turbo.build/schema.json",
+  "globalDependencies": ["global_deps/**"],
+  "globalEnv": ["SOME_ENV_VAR"],
+  "pipeline": {
+    "build": {
+      "env": ["NODE_ENV"],
+      "outputs": []
+    },
+    // this comment verifies that turbo can read .json files with comments
+    "my-app#build": {
+      "outputs": ["banana.txt", "apple.json"],
+      "dotEnv": [".env.local"]
+    },
+
+    "something": {},
+    "//#something": {},
+
+    "maybefails": {}
+  }
+}

turborepo-tests/integration/tests/global-deps.t

@@ -0,0 +1,26 @@
+Setup
+  $ . ${TESTDIR}/../../helpers/setup_integration_test.sh global_deps
+
+Run a build
+  $ ${TURBO} build -F my-app --output-logs=hash-only
+  \xe2\x80\xa2 Packages in scope: my-app (esc)
+  \xe2\x80\xa2 Running build in 1 packages (esc)
+  \xe2\x80\xa2 Remote caching disabled (esc)
+  my-app:build: cache miss, executing 81165ceb4ed0e31f
+
+   Tasks:    1 successful, 1 total
+  Cached:    0 cached, 1 total
+    Time:\s*[\.0-9]+m?s  (re)
+
+
+  $ echo "new text" > global_deps/foo.txt
+  $ ${TURBO} build -F my-app --output-logs=hash-only
+  \xe2\x80\xa2 Packages in scope: my-app (esc)
+  \xe2\x80\xa2 Running build in 1 packages (esc)
+  \xe2\x80\xa2 Remote caching disabled (esc)
+  my-app:build: cache miss, executing 7bc88ba3e84628fd
+
+   Tasks:    1 successful, 1 total
+  Cached:    0 cached, 1 total
+    Time:\s*[\.0-9]+m?s  (re)
+